Message-ID: <PH0PR21MB3025C19EDC8F5230DB1957EBD7169@PH0PR21MB3025.namprd21.prod.outlook.com>
Date:   Mon, 21 Mar 2022 18:23:13 +0000
From:   "Michael Kelley (LINUX)" <mikelley@...rosoft.com>
To:     Andrea Parri <parri.andrea@...il.com>
CC:     KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Wei Liu <wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>,
        Wei Hu <weh@...rosoft.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Wilczynski <kw@...ux.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/2] PCI: hv: Use IDR to generate transaction IDs for
 VMBus hardening

From: Andrea Parri <parri.andrea@...il.com> Sent: Sunday, March 20, 2022 7:59 AM
> 
> On Sat, Mar 19, 2022 at 04:20:13PM +0000, Michael Kelley (LINUX) wrote:
> > From: Andrea Parri (Microsoft) <parri.andrea@...il.com> Sent: Friday, March 18,
> 2022 10:49 AM
> > >
> > > Currently, pointers to guest memory are passed to Hyper-V as transaction
> > > IDs in hv_pci.  In the face of errors or malicious behavior in Hyper-V,
> > > hv_pci should not expose or trust the transaction IDs returned by
> > > Hyper-V to be valid guest memory addresses.  Instead, use small integers
> > > generated by IDR as request (transaction) IDs.
> >
> > I had expected that this code would use the next_request_id_callback
> > mechanism because of the race conditions that mechanism solves.  And
> > to protect against a malicious Hyper-V sending a bogus second message
> > with the same requestID, the requestID needs to be freed in the
> > onchannelcallback function as is done with vmbus_request_addr().
> 
> I think I should elaborate on the design underlying this submission;
> roughly, the present solution diverges from the 'generic' requestor
> mechanism you mentioned above in two main aspects:
> 
>   A) it 'moves' the ID removal into hv_compose_msi_msg() and other
>      functions,

Right.  A key implication is that this patch allows the completion
function to be called multiple times, if Hyper-V were to be malicious
and send multiple responses with the same requestID.  This is OK as
long as the completion functions are idempotent, which after looking,
I think they are in this driver.

Furthermore, this patch allows the completion function to run anytime
between when the requestID is created and when it is deleted.  This
patch creates the requestID just before calling vmbus_sendpacket(),
which is good.  The requestID is deleted later in the various functions.
I saw only one potential problem, which is in new_pcichild_device(),
where the new hpdev is added to a global list before the requestID is
deleted. There's a window where the completion function could run
and update the probed_bar[] values asynchronously after the hpdev is
on the global list.  I don't know if this is a problem or not, but it could
be prevented by deleting the requestID a little earlier in the function.

> 
>   B) it adopts some ad-hoc locking scheme in the channel callback.
> 
> AFAICT, such changes preserve the 'confidentiality' and correctness
> guarantees of the generic approach (modulo the issue discussed here
> with Saurabh).

Yes, I agree, assuming the current functionality of the completion
functions.

> 
> These changes are justified by the bug/fix discussed in 2/2.  For
> concreteness, consider a solution based on the VMbus requestor as
> reported at the end of this email.
> 
> AFAICT, this solution can't fix the bug discussed in 2/2.  Moreover
> (and looking back at (A-B)), we observe that:
> 
>   1) locking in the channel callback is not quite as desired: we'd
>      want a request_addr_callback_nolock() say and 'protected' it
>      together with ->completion_func();

I'm not understanding this point.  Could you clarify?

> 
>   2) hv_compose_msi_msg() doesn't know the value of the request ID
>      it has allocated (hv_compose_msi_msg() -> vmbus_sendpacket();
>      cf. also remove_request_id() in the current submission).

Agreed.  This would have to be addressed by adding another version of
vmbus_sendpacket() that returns the request ID.

> 
> Hope this helps clarify the problems at stake, and move forward to a
> 'final' solution...

I think there's a reasonable way for the vmbus_next_request_id()
mechanism to solve the problem in Patch 2/2 (if a new version of
vmbus_sendpacket is added).  To me, that mechanism seems safer
in that it restricts the completion function to running just once
per requestID.  With this patch, we must remember that the
completion functions must remain idempotent.

But I can go either way.  I can give an OK on this solution if that's
the preferred path.  Other input is also welcome ...

Michael

> 
> Thanks,
>   Andrea
> 
> 
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index ae0bc2fee4ca8..bd99dd12d367b 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -91,6 +91,9 @@ static enum pci_protocol_version_t pci_protocol_versions[] = {
>  /* space for 32bit serial number as string */
>  #define SLOT_NAME_SIZE 11
> 
> +/* Size of requestor for VMbus */
> +#define HV_PCI_RQSTOR_SIZE 64
> +
>  /*
>   * Message Types
>   */
> @@ -1407,7 +1410,7 @@ static void hv_int_desc_free(struct hv_pci_dev *hpdev,
>  	int_pkt->wslot.slot = hpdev->desc.win_slot.slot;
>  	int_pkt->int_desc = *int_desc;
>  	vmbus_sendpacket(hpdev->hbus->hdev->channel, int_pkt, sizeof(*int_pkt),
> -			 (unsigned long)&ctxt.pkt, VM_PKT_DATA_INBAND, 0);
> +			 0, VM_PKT_DATA_INBAND, 0);
>  	kfree(int_desc);
>  }
> 
> @@ -2649,7 +2652,7 @@ static void hv_eject_device_work(struct work_struct *work)
>  	ejct_pkt->message_type.type = PCI_EJECTION_COMPLETE;
>  	ejct_pkt->wslot.slot = hpdev->desc.win_slot.slot;
>  	vmbus_sendpacket(hbus->hdev->channel, ejct_pkt,
> -			 sizeof(*ejct_pkt), (unsigned long)&ctxt.pkt,
> +			 sizeof(*ejct_pkt), 0,
>  			 VM_PKT_DATA_INBAND, 0);
> 
>  	/* For the get_pcichild() in hv_pci_eject_device() */
> @@ -2696,8 +2699,9 @@ static void hv_pci_onchannelcallback(void *context)
>  	const int packet_size = 0x100;
>  	int ret;
>  	struct hv_pcibus_device *hbus = context;
> +	struct vmbus_channel *chan = hbus->hdev->channel;
>  	u32 bytes_recvd;
> -	u64 req_id;
> +	u64 req_id, req_addr;
>  	struct vmpacket_descriptor *desc;
>  	unsigned char *buffer;
>  	int bufferlen = packet_size;
> @@ -2743,11 +2747,13 @@ static void hv_pci_onchannelcallback(void *context)
>  		switch (desc->type) {
>  		case VM_PKT_COMP:
> 
> -			/*
> -			 * The host is trusted, and thus it's safe to interpret
> -			 * this transaction ID as a pointer.
> -			 */
> -			comp_packet = (struct pci_packet *)req_id;
> +			req_addr = chan->request_addr_callback(chan, req_id);
> +			if (!req_addr || req_addr == VMBUS_RQST_ERROR) {
> +				dev_warn_ratelimited(&hbus->hdev->device,
> +						     "Invalid request ID\n");
> +				break;
> +			}
> +			comp_packet = (struct pci_packet *)req_addr;
>  			response = (struct pci_response *)buffer;
>  			comp_packet->completion_func(comp_packet->compl_ctxt,
>  						     response,
> @@ -3419,6 +3425,10 @@ static int hv_pci_probe(struct hv_device *hdev,
>  		goto free_dom;
>  	}
> 
> +	hdev->channel->next_request_id_callback = vmbus_next_request_id;
> +	hdev->channel->request_addr_callback = vmbus_request_addr;
> +	hdev->channel->rqstor_size = HV_PCI_RQSTOR_SIZE;
> +
>  	ret = vmbus_open(hdev->channel, pci_ring_size, pci_ring_size, NULL, 0,
>  			 hv_pci_onchannelcallback, hbus);
>  	if (ret)
> @@ -3749,6 +3759,10 @@ static int hv_pci_resume(struct hv_device *hdev)
> 
>  	hbus->state = hv_pcibus_init;
> 
> +	hdev->channel->next_request_id_callback = vmbus_next_request_id;
> +	hdev->channel->request_addr_callback = vmbus_request_addr;
> +	hdev->channel->rqstor_size = HV_PCI_RQSTOR_SIZE;
> +
>  	ret = vmbus_open(hdev->channel, pci_ring_size, pci_ring_size, NULL, 0,
>  			 hv_pci_onchannelcallback, hbus);
>  	if (ret)

Powered by blists - more mailing lists