| Message ID | 20220504125039.2598-2-parri.andrea@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: hv: (More) Hardening changes | expand |
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
> 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
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) {
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(-)