[PATCH 0/2] PCI: hv: (More) Hardening changes

[PATCH 0/2] PCI: hv: (More) Hardening changes

[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

[PATCH 1/2] PCI: hv: Add validation for untrusted Hyper-V values

[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

[PATCH 2/2] PCI: hv: Fix synchronization between channel callback and hv_pci_bus_exit()

[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

RE: [PATCH 1/2] PCI: hv: Add validation for untrusted Hyper-V values

[Michael Kelley (LINUX)]

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

RE: [PATCH 2/2] PCI: hv: Fix synchronization between channel callback and hv_pci_bus_exit()

[Michael Kelley (LINUX)]

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>

Re: [PATCH 1/2] PCI: hv: Add validation for untrusted Hyper-V values

[Andrea Parri]

> 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