linux-kernel - Re: [PATCH v9 24/27] virt: gunyah: Add proxy-scheduled vCPUs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230209103955.GJ332@quicinc.com>
Date:   Thu, 9 Feb 2023 16:09:55 +0530
From:   Srivatsa Vaddagiri <quic_svaddagi@...cinc.com>
To:     Elliot Berman <quic_eberman@...cinc.com>
CC:     Bjorn Andersson <quic_bjorande@...cinc.com>,
        Alex Elder <elder@...aro.org>,
        Murali Nalajala <quic_mnalajal@...cinc.com>,
        "Jonathan Corbet" <corbet@....net>,
        Catalin Marinas <catalin.marinas@....com>,
        "Will Deacon" <will@...nel.org>,
        Trilok Soni <quic_tsoni@...cinc.com>,
        "Carl van Schaik" <quic_cvanscha@...cinc.com>,
        Prakruthi Deepak Heragu <quic_pheragu@...cinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh+dt@...nel.org>,
        "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@...aro.org>,
        Bagas Sanjaya <bagasdotme@...il.com>,
        Marc Zyngier <maz@...nel.org>,
        Jassi Brar <jassisinghbrar@...il.com>,
        Sudeep Holla <sudeep.holla@....com>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-doc@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v9 24/27] virt: gunyah: Add proxy-scheduled vCPUs

* Elliot Berman <quic_eberman@...cinc.com> [2023-01-20 14:46:23]:

> +static void gh_handle_mmio_return(struct gunyah_vcpu *vcpu, u64 *state)
> +{
> +	if (!vcpu->vcpu_run->mmio.is_write)
> +		memcpy(&state[0], vcpu->vcpu_run->mmio.data, vcpu->vcpu_run->mmio.len);

Would be good to do a bound check on length of memcpy I think (in case 
vcpu_run_resp->state_data[1] is wrong for example).

> +
> +	vcpu->handle_mmio = false;
> +	vcpu->vcpu_run->exit_reason = GH_VM_EXIT_UNKNOWN;
> +}
> +

// snip

> +static int gh_vcpu_run(struct gunyah_vcpu *vcpu)
> +{
> +	struct gh_hypercall_vcpu_run_resp vcpu_run_resp;
> +	u64 state_data[3] = { 0 };
> +	int ret = 0;
> +
> +	ret = gh_vm_ensure_started(vcpu->ghvm);
> +	if (ret)
> +		return ret;

Can we move this to VM_START ioctl and avoid this check in fast path? In case VM
is not started, then I think gh_hypercall_vcpu_run() will fail which can catch
erroneous use of VCPU_RUN w/o a preceding VM_START. Alternately we could use a
flag in vcpu struct to check for this case (similar to test for vcpu->rsc
below).

// snip

> +			case GH_VCPU_STATE_EXPECTS_WAKEUP:
> +			case GH_VCPU_STATE_POWERED_OFF:
> +				ret = wait_for_completion_interruptible(&vcpu->ready);

I think we should end this wait in case immediate_exit is set as well.

> +static vm_fault_t gh_vcpu_fault(struct vm_fault *vmf)
> +{
> +	struct gunyah_vcpu *vcpu = vmf->vma->vm_file->private_data;
> +	struct page *page = NULL;
> +
> +	if (vmf->pgoff == 0)
> +		page = virt_to_page(vcpu->vcpu_run);
> +
> +	get_page(page);

We should avoid get_page in case page is NULL.

> +	vmf->page = page;
> +	return 0;
> +}
> +
> +static void gunyah_vcpu_unpopulate(struct gunyah_vm_resource_ticket *ticket,
> +				   struct gunyah_resource *ghrsc)
> +{
> +	struct gunyah_vcpu *vcpu = container_of(ticket, struct gunyah_vcpu, ticket);
> +
> +	vcpu->vcpu_run->immediate_exit = true;

We should poke the vcpu thread as well so that it can notice this.
Otherwise it can continue to be in gh_hypercall_vcpu_run() or
wait_for_completion_interruptible() for longer time to come.

> +	mutex_lock(&vcpu->run_lock);
> +	free_irq(vcpu->rsc->irq, vcpu);
> +	vcpu->rsc = NULL;
> +	mutex_unlock(&vcpu->run_lock);
> +}
> +
> +static long gunyah_vcpu_bind(struct gunyah_vm_function *f)
> +{
> +	struct gunyah_vcpu *vcpu;
> +	char name[MAX_VCPU_NAME];
> +	struct file *file;
> +	struct page *page;
> +	int fd;
> +	long r;
> +
> +	if (!gh_api_has_feature(GH_API_FEATURE_VCPU))

We should test for this feature before registering the function? What's
the point in registering a function otherwise if it can't do its work!