* [PATCH 0/2] PCI: hv: (More) Hardening changes
@ 2022-05-04 12:50 Andrea Parri (Microsoft)
2022-05-04 12:50 ` [PATCH 1/2] PCI: hv: Add validation for untrusted Hyper-V values Andrea Parri (Microsoft)
2022-05-04 12:50 ` [PATCH 2/2] PCI: hv: Fix synchronization between channel callback and hv_pci_bus_exit() Andrea Parri (Microsoft)
0 siblings, 2 replies; 6+ messages in thread
From: Andrea Parri (Microsoft) @ 2022-05-04 12:50 UTC (permalink / raw)
To: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Dexuan Cui, Michael Kelley, Lorenzo Pieralisi, Rob Herring,
Krzysztof Wilczynski, Bjorn Helgaas
Cc: linux-hyperv, linux-pci, linux-kernel, Andrea Parri (Microsoft)
Patch #2 depends on changes in hyperv-next. (No urgency here: will
resend/rebase after 5.19-rc1 if desired, just let me know...)
Thanks,
Andrea
Andrea Parri (Microsoft) (2):
PCI: hv: Add validation for untrusted Hyper-V values
PCI: hv: Fix synchronization between channel callback and
hv_pci_bus_exit()
drivers/pci/controller/pci-hyperv.c | 53 +++++++++++++++++++++--------
1 file changed, 39 insertions(+), 14 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] PCI: hv: Add validation for untrusted Hyper-V values 2022-05-04 12:50 [PATCH 0/2] PCI: hv: (More) Hardening changes Andrea Parri (Microsoft) @ 2022-05-04 12:50 ` Andrea Parri (Microsoft) 2022-05-05 21:59 ` Michael Kelley (LINUX) 2022-05-04 12:50 ` [PATCH 2/2] PCI: hv: Fix synchronization between channel callback and hv_pci_bus_exit() Andrea Parri (Microsoft) 1 sibling, 1 reply; 6+ messages in thread From: Andrea Parri (Microsoft) @ 2022-05-04 12:50 UTC (permalink / raw) To: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu, Dexuan Cui, Michael Kelley, Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczynski, Bjorn Helgaas Cc: linux-hyperv, linux-pci, linux-kernel, Andrea Parri (Microsoft) For additional robustness in the face of Hyper-V errors or malicious behavior, validate all values that originate from packets that Hyper-V has sent to the guest in the host-to-guest ring buffer. Ensure that invalid values cannot cause data being copied out of the bounds of the source buffer in hv_pci_onchannelcallback(). While at it, remove a redundant validation in hv_pci_generic_compl(): hv_pci_onchannelcallback() already ensures that all processed incoming packets are "at least as large as [in fact larger than] a response". Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com> --- drivers/pci/controller/pci-hyperv.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c index cf2fe5754fde4..9a3e17b682eb7 100644 --- a/drivers/pci/controller/pci-hyperv.c +++ b/drivers/pci/controller/pci-hyperv.c @@ -981,11 +981,7 @@ static void hv_pci_generic_compl(void *context, struct pci_response *resp, { struct hv_pci_compl *comp_pkt = context; - if (resp_packet_size >= offsetofend(struct pci_response, status)) - comp_pkt->completion_status = resp->status; - else - comp_pkt->completion_status = -1; - + comp_pkt->completion_status = resp->status; complete(&comp_pkt->host_event); } @@ -1602,8 +1598,13 @@ static void hv_pci_compose_compl(void *context, struct pci_response *resp, struct pci_create_int_response *int_resp = (struct pci_create_int_response *)resp; + if (resp_packet_size < sizeof(*int_resp)) { + comp_pkt->comp_pkt.completion_status = -1; + goto out; + } comp_pkt->comp_pkt.completion_status = resp->status; comp_pkt->int_desc = int_resp->int_desc; +out: complete(&comp_pkt->comp_pkt.host_event); } @@ -2806,7 +2807,8 @@ static void hv_pci_onchannelcallback(void *context) case PCI_BUS_RELATIONS: bus_rel = (struct pci_bus_relations *)buffer; - if (bytes_recvd < + if (bytes_recvd < sizeof(*bus_rel) || + bytes_recvd < struct_size(bus_rel, func, bus_rel->device_count)) { dev_err(&hbus->hdev->device, @@ -2820,7 +2822,8 @@ static void hv_pci_onchannelcallback(void *context) case PCI_BUS_RELATIONS2: bus_rel2 = (struct pci_bus_relations2 *)buffer; - if (bytes_recvd < + if (bytes_recvd < sizeof(*bus_rel2) || + bytes_recvd < struct_size(bus_rel2, func, bus_rel2->device_count)) { dev_err(&hbus->hdev->device, @@ -2834,6 +2837,11 @@ static void hv_pci_onchannelcallback(void *context) case PCI_EJECT: dev_message = (struct pci_dev_incoming *)buffer; + if (bytes_recvd < sizeof(*dev_message)) { + dev_err(&hbus->hdev->device, + "eject message too small\n"); + break; + } hpdev = get_pcichild_wslot(hbus, dev_message->wslot.slot); if (hpdev) { @@ -2845,6 +2853,11 @@ static void hv_pci_onchannelcallback(void *context) case PCI_INVALIDATE_BLOCK: inval = (struct pci_dev_inval_block *)buffer; + if (bytes_recvd < sizeof(*inval)) { + dev_err(&hbus->hdev->device, + "invalidate message too small\n"); + break; + } hpdev = get_pcichild_wslot(hbus, inval->wslot.slot); if (hpdev) { -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH 1/2] PCI: hv: Add validation for untrusted Hyper-V values 2022-05-04 12:50 ` [PATCH 1/2] PCI: hv: Add validation for untrusted Hyper-V values Andrea Parri (Microsoft) @ 2022-05-05 21:59 ` Michael Kelley (LINUX) 2022-05-06 7:47 ` Andrea Parri 0 siblings, 1 reply; 6+ messages in thread From: Michael Kelley (LINUX) @ 2022-05-05 21:59 UTC (permalink / raw) To: Andrea Parri (Microsoft), KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu, Dexuan Cui, Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczynski, Bjorn Helgaas Cc: linux-hyperv, linux-pci, linux-kernel From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, May 4, 2022 5:51 AM > > For additional robustness in the face of Hyper-V errors or malicious > behavior, validate all values that originate from packets that Hyper-V > has sent to the guest in the host-to-guest ring buffer. Ensure that > invalid values cannot cause data being copied out of the bounds of the > source buffer in hv_pci_onchannelcallback(). > > While at it, remove a redundant validation in hv_pci_generic_compl(): > hv_pci_onchannelcallback() already ensures that all processed incoming > packets are "at least as large as [in fact larger than] a response". > > Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com> > --- > drivers/pci/controller/pci-hyperv.c | 27 ++++++++++++++++++++------- > 1 file changed, 20 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > index cf2fe5754fde4..9a3e17b682eb7 100644 > --- a/drivers/pci/controller/pci-hyperv.c > +++ b/drivers/pci/controller/pci-hyperv.c > @@ -981,11 +981,7 @@ static void hv_pci_generic_compl(void *context, struct > pci_response *resp, > { > struct hv_pci_compl *comp_pkt = context; > > - if (resp_packet_size >= offsetofend(struct pci_response, status)) > - comp_pkt->completion_status = resp->status; > - else > - comp_pkt->completion_status = -1; > - > + comp_pkt->completion_status = resp->status; > complete(&comp_pkt->host_event); > } > > @@ -1602,8 +1598,13 @@ static void hv_pci_compose_compl(void *context, struct > pci_response *resp, > struct pci_create_int_response *int_resp = > (struct pci_create_int_response *)resp; > > + if (resp_packet_size < sizeof(*int_resp)) { > + comp_pkt->comp_pkt.completion_status = -1; > + goto out; > + } > comp_pkt->comp_pkt.completion_status = resp->status; > comp_pkt->int_desc = int_resp->int_desc; > +out: > complete(&comp_pkt->comp_pkt.host_event); > } > > @@ -2806,7 +2807,8 @@ static void hv_pci_onchannelcallback(void *context) > case PCI_BUS_RELATIONS: > > bus_rel = (struct pci_bus_relations *)buffer; > - if (bytes_recvd < > + if (bytes_recvd < sizeof(*bus_rel) || > + bytes_recvd < > struct_size(bus_rel, func, > bus_rel->device_count)) { > dev_err(&hbus->hdev->device, > @@ -2820,7 +2822,8 @@ static void hv_pci_onchannelcallback(void *context) > case PCI_BUS_RELATIONS2: > > bus_rel2 = (struct pci_bus_relations2 *)buffer; > - if (bytes_recvd < > + if (bytes_recvd < sizeof(*bus_rel2) || > + bytes_recvd < > struct_size(bus_rel2, func, > bus_rel2->device_count)) { > dev_err(&hbus->hdev->device, > @@ -2834,6 +2837,11 @@ static void hv_pci_onchannelcallback(void *context) > case PCI_EJECT: > > dev_message = (struct pci_dev_incoming *)buffer; > + if (bytes_recvd < sizeof(*dev_message)) { > + dev_err(&hbus->hdev->device, > + "eject message too small\n"); > + break; > + } > hpdev = get_pcichild_wslot(hbus, > dev_message->wslot.slot); > if (hpdev) { > @@ -2845,6 +2853,11 @@ static void hv_pci_onchannelcallback(void *context) > case PCI_INVALIDATE_BLOCK: > > inval = (struct pci_dev_inval_block *)buffer; > + if (bytes_recvd < sizeof(*inval)) { > + dev_err(&hbus->hdev->device, > + "invalidate message too small\n"); > + break; > + } > hpdev = get_pcichild_wslot(hbus, > inval->wslot.slot); > if (hpdev) { > -- > 2.25.1 I don't see any issues with the code here. But check the function q_resource_requirements(). Doesn't it need the same treatment as you've done above with hv_pci_compose_compl()? For completeness, the fix for q_resource_requirements() should be included in this patch as well. Michael ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] PCI: hv: Add validation for untrusted Hyper-V values 2022-05-05 21:59 ` Michael Kelley (LINUX) @ 2022-05-06 7:47 ` Andrea Parri 0 siblings, 0 replies; 6+ messages in thread From: Andrea Parri @ 2022-05-06 7:47 UTC (permalink / raw) To: Michael Kelley (LINUX) Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu, Dexuan Cui, Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczynski, Bjorn Helgaas, linux-hyperv, linux-pci, linux-kernel > I don't see any issues with the code here. But check the function > q_resource_requirements(). Doesn't it need the same treatment as you've > done above with hv_pci_compose_compl()? For completeness, the > fix for q_resource_requirements() should be included in this patch as well. Yes, indeed. Will do for v2. Thanks, Andrea ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] PCI: hv: Fix synchronization between channel callback and hv_pci_bus_exit() 2022-05-04 12:50 [PATCH 0/2] PCI: hv: (More) Hardening changes Andrea Parri (Microsoft) 2022-05-04 12:50 ` [PATCH 1/2] PCI: hv: Add validation for untrusted Hyper-V values Andrea Parri (Microsoft) @ 2022-05-04 12:50 ` Andrea Parri (Microsoft) 2022-05-05 22:00 ` Michael Kelley (LINUX) 1 sibling, 1 reply; 6+ messages in thread From: Andrea Parri (Microsoft) @ 2022-05-04 12:50 UTC (permalink / raw) To: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu, Dexuan Cui, Michael Kelley, Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczynski, Bjorn Helgaas Cc: linux-hyperv, linux-pci, linux-kernel, Andrea Parri (Microsoft) [ Similarly to commit a765ed47e4516 ("PCI: hv: Fix synchronization between channel callback and hv_compose_msi_msg()"): ] The (on-stack) teardown packet becomes invalid once the completion timeout in hv_pci_bus_exit() has expired and hv_pci_bus_exit() has returned. Prevent the channel callback from accessing the invalid packet by removing the ID associated to such packet from the VMbus requestor in hv_pci_bus_exit(). Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com> --- drivers/pci/controller/pci-hyperv.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c index 9a3e17b682eb7..db4b3f86726b2 100644 --- a/drivers/pci/controller/pci-hyperv.c +++ b/drivers/pci/controller/pci-hyperv.c @@ -3620,6 +3620,7 @@ static int hv_pci_probe(struct hv_device *hdev, static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs) { struct hv_pcibus_device *hbus = hv_get_drvdata(hdev); + struct vmbus_channel *chan = hdev->channel; struct { struct pci_packet teardown_packet; u8 buffer[sizeof(struct pci_message)]; @@ -3627,13 +3628,14 @@ static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs) struct hv_pci_compl comp_pkt; struct hv_pci_dev *hpdev, *tmp; unsigned long flags; + u64 trans_id; int ret; /* * After the host sends the RESCIND_CHANNEL message, it doesn't * access the per-channel ringbuffer any longer. */ - if (hdev->channel->rescind) + if (chan->rescind) return 0; if (!keep_devs) { @@ -3670,16 +3672,26 @@ static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs) pkt.teardown_packet.compl_ctxt = &comp_pkt; pkt.teardown_packet.message[0].type = PCI_BUS_D0EXIT; - ret = vmbus_sendpacket(hdev->channel, &pkt.teardown_packet.message, - sizeof(struct pci_message), - (unsigned long)&pkt.teardown_packet, - VM_PKT_DATA_INBAND, - VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED); + ret = vmbus_sendpacket_getid(chan, &pkt.teardown_packet.message, + sizeof(struct pci_message), + (unsigned long)&pkt.teardown_packet, + &trans_id, VM_PKT_DATA_INBAND, + VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED); if (ret) return ret; - if (wait_for_completion_timeout(&comp_pkt.host_event, 10 * HZ) == 0) + if (wait_for_completion_timeout(&comp_pkt.host_event, 10 * HZ) == 0) { + /* + * The completion packet on the stack becomes invalid after + * 'return'; remove the ID from the VMbus requestor if the + * identifier is still mapped to/associated with the packet. + * + * Cf. hv_pci_onchannelcallback(). + */ + vmbus_request_addr_match(chan, trans_id, + (unsigned long)&pkt.teardown_packet); return -ETIMEDOUT; + } return 0; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH 2/2] PCI: hv: Fix synchronization between channel callback and hv_pci_bus_exit() 2022-05-04 12:50 ` [PATCH 2/2] PCI: hv: Fix synchronization between channel callback and hv_pci_bus_exit() Andrea Parri (Microsoft) @ 2022-05-05 22:00 ` Michael Kelley (LINUX) 0 siblings, 0 replies; 6+ messages in thread From: Michael Kelley (LINUX) @ 2022-05-05 22:00 UTC (permalink / raw) To: Andrea Parri (Microsoft), KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu, Dexuan Cui, Lorenzo Pieralisi, Rob Herring, Krzysztof Wilczynski, Bjorn Helgaas Cc: linux-hyperv, linux-pci, linux-kernel From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, May 4, 2022 5:51 AM > > [ Similarly to commit a765ed47e4516 ("PCI: hv: Fix synchronization > between channel callback and hv_compose_msi_msg()"): ] > > The (on-stack) teardown packet becomes invalid once the completion > timeout in hv_pci_bus_exit() has expired and hv_pci_bus_exit() has > returned. Prevent the channel callback from accessing the invalid > packet by removing the ID associated to such packet from the VMbus > requestor in hv_pci_bus_exit(). > > Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com> > --- > drivers/pci/controller/pci-hyperv.c | 26 +++++++++++++++++++------- > 1 file changed, 19 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > index 9a3e17b682eb7..db4b3f86726b2 100644 > --- a/drivers/pci/controller/pci-hyperv.c > +++ b/drivers/pci/controller/pci-hyperv.c > @@ -3620,6 +3620,7 @@ static int hv_pci_probe(struct hv_device *hdev, > static int hv_pci_bus_exit(struct hv_device *hdev, bool keep_devs) > { > struct hv_pcibus_device *hbus = hv_get_drvdata(hdev); > + struct vmbus_channel *chan = hdev->channel; > struct { > struct pci_packet teardown_packet; > u8 buffer[sizeof(struct pci_message)]; > @@ -3627,13 +3628,14 @@ static int hv_pci_bus_exit(struct hv_device *hdev, bool > keep_devs) > struct hv_pci_compl comp_pkt; > struct hv_pci_dev *hpdev, *tmp; > unsigned long flags; > + u64 trans_id; > int ret; > > /* > * After the host sends the RESCIND_CHANNEL message, it doesn't > * access the per-channel ringbuffer any longer. > */ > - if (hdev->channel->rescind) > + if (chan->rescind) > return 0; > > if (!keep_devs) { > @@ -3670,16 +3672,26 @@ static int hv_pci_bus_exit(struct hv_device *hdev, bool > keep_devs) > pkt.teardown_packet.compl_ctxt = &comp_pkt; > pkt.teardown_packet.message[0].type = PCI_BUS_D0EXIT; > > - ret = vmbus_sendpacket(hdev->channel, &pkt.teardown_packet.message, > - sizeof(struct pci_message), > - (unsigned long)&pkt.teardown_packet, > - VM_PKT_DATA_INBAND, > - VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED); > + ret = vmbus_sendpacket_getid(chan, &pkt.teardown_packet.message, > + sizeof(struct pci_message), > + (unsigned long)&pkt.teardown_packet, > + &trans_id, VM_PKT_DATA_INBAND, > + > VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED); > if (ret) > return ret; > > - if (wait_for_completion_timeout(&comp_pkt.host_event, 10 * HZ) == 0) > + if (wait_for_completion_timeout(&comp_pkt.host_event, 10 * HZ) == 0) { > + /* > + * The completion packet on the stack becomes invalid after > + * 'return'; remove the ID from the VMbus requestor if the > + * identifier is still mapped to/associated with the packet. > + * > + * Cf. hv_pci_onchannelcallback(). > + */ > + vmbus_request_addr_match(chan, trans_id, > + (unsigned long)&pkt.teardown_packet); > return -ETIMEDOUT; > + } > > return 0; > } > -- > 2.25.1 Reviewed-by: Michael Kelley <mikelley@microsoft.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-05-06 7:48 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-04 12:50 [PATCH 0/2] PCI: hv: (More) Hardening changes Andrea Parri (Microsoft) 2022-05-04 12:50 ` [PATCH 1/2] PCI: hv: Add validation for untrusted Hyper-V values Andrea Parri (Microsoft) 2022-05-05 21:59 ` Michael Kelley (LINUX) 2022-05-06 7:47 ` Andrea Parri 2022-05-04 12:50 ` [PATCH 2/2] PCI: hv: Fix synchronization between channel callback and hv_pci_bus_exit() Andrea Parri (Microsoft) 2022-05-05 22:00 ` Michael Kelley (LINUX)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).