From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9DF95C433EF for ; Mon, 25 Apr 2022 16:50:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240687AbiDYQxz (ORCPT ); Mon, 25 Apr 2022 12:53:55 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50728 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232888AbiDYQxx (ORCPT ); Mon, 25 Apr 2022 12:53:53 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id F3DA8DE8F; Mon, 25 Apr 2022 09:50:47 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id BC45F1FB; Mon, 25 Apr 2022 09:50:47 -0700 (PDT) Received: from lpieralisi (unknown [10.57.13.157]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id AB9873F5A1; Mon, 25 Apr 2022 09:50:44 -0700 (PDT) Date: Mon, 25 Apr 2022 17:50:37 +0100 From: Lorenzo Pieralisi To: "Andrea Parri (Microsoft)" Cc: KY Srinivasan , Haiyang Zhang , Stephen Hemminger , Wei Liu , Dexuan Cui , Michael Kelley , Wei Hu , Rob Herring , Krzysztof Wilczynski , Bjorn Helgaas , linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 2/6] PCI: hv: Use vmbus_requestor to generate transaction IDs for VMbus hardening Message-ID: <20220425165037.GA17718@lpieralisi> References: <20220419122325.10078-1-parri.andrea@gmail.com> <20220419122325.10078-3-parri.andrea@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220419122325.10078-3-parri.andrea@gmail.com> User-Agent: Mutt/1.9.4 (2018-02-28) Precedence: bulk List-ID: X-Mailing-List: linux-hyperv@vger.kernel.org On Tue, Apr 19, 2022 at 02:23:21PM +0200, Andrea Parri (Microsoft) wrote: > 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 vmbus_requestor as request (transaction) IDs. > > Suggested-by: Michael Kelley > Signed-off-by: Andrea Parri (Microsoft) > --- > drivers/pci/controller/pci-hyperv.c | 39 +++++++++++++++++++++-------- > 1 file changed, 29 insertions(+), 10 deletions(-) Acked-by: Lorenzo Pieralisi > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > index 88b3b56d05228..0252b4bbc8d15 100644 > --- a/drivers/pci/controller/pci-hyperv.c > +++ b/drivers/pci/controller/pci-hyperv.c > @@ -91,6 +91,13 @@ 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; the value is based on the observation > + * that having more than one request outstanding is 'rare', and so 64 > + * should be generous in ensuring that we don't ever run out. > + */ > +#define HV_PCI_RQSTOR_SIZE 64 > + > /* > * Message Types > */ > @@ -1407,7 +1414,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 +2656,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 +2703,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; > @@ -2715,8 +2723,8 @@ static void hv_pci_onchannelcallback(void *context) > return; > > while (1) { > - ret = vmbus_recvpacket_raw(hbus->hdev->channel, buffer, > - bufferlen, &bytes_recvd, &req_id); > + ret = vmbus_recvpacket_raw(chan, buffer, bufferlen, > + &bytes_recvd, &req_id); > > if (ret == -ENOBUFS) { > kfree(buffer); > @@ -2743,11 +2751,14 @@ 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 == VMBUS_RQST_ERROR) { > + dev_err(&hbus->hdev->device, > + "Invalid transaction ID %llx\n", > + req_id); > + break; > + } > + comp_packet = (struct pci_packet *)req_addr; > response = (struct pci_response *)buffer; > comp_packet->completion_func(comp_packet->compl_ctxt, > response, > @@ -3428,6 +3439,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) > @@ -3758,6 +3773,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) > -- > 2.25.1 >