* [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
* [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 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 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
* 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
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).