[PATCH 0/3] scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback() -- Take 2

[PATCH 0/3] scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback() -- Take 2

[Andrea Parri (Microsoft)]

Hi all,

This series is to address the problems mentioned in:

  4da3a54f5a0258 ("Revert "scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback()"")

(cf., in particular, patch 2/3) and to re-introduce the validation in
question (patch 3/3); patch 1/3 emerged from internal review of these
two patches and is a related fix.

Thanks,
  Andrea


Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org

Andrea Parri (Microsoft) (3):
  scsi: storvsc: Fix max_outstanding_req_per_channel for Win8 and newer
  scsi: storvsc: Resolve data race in storvsc_probe()
  scsi: storvsc: Validate length of incoming packet in
    storvsc_on_channel_callback()

 drivers/scsi/storvsc_drv.c | 58 +++++++++++++++++++++++---------------
 1 file changed, 36 insertions(+), 22 deletions(-)

-- 
2.25.1

[PATCH 1/3] scsi: storvsc: Fix max_outstanding_req_per_channel for Win8 and newer

[Andrea Parri (Microsoft)]

Current code overestimates the value of max_outstanding_req_per_channel
for Win8 and newer hosts, since vmscsi_size_delta is set to the initial
value of sizeof(vmscsi_win8_extension) rather than zero.  This may lead
to wrong decisions when using ring_avail_percent_lowater equals to zero.
The estimate of max_outstanding_req_per_channel is 'exact' for Win7 and
older hosts.  A better choice, keeping the algorithm for the estimation
simple, is to err the other way around, i.e., to underestimate for Win7
and older but to use the exact value for Win8 and newer.

Suggested-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/storvsc_drv.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index ded00a89bfc4e..64298aa2f151e 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -2141,12 +2141,15 @@ static int __init storvsc_drv_init(void)
 	 * than the ring buffer size since that page is reserved for
 	 * the ring buffer indices) by the max request size (which is
 	 * vmbus_channel_packet_multipage_buffer + struct vstor_packet + u64)
+	 *
+	 * The computation underestimates max_outstanding_req_per_channel
+	 * for Win7 and older hosts because it does not take into account
+	 * the vmscsi_size_delta correction to the max request size.
 	 */
 	max_outstanding_req_per_channel =
 		((storvsc_ringbuffer_size - PAGE_SIZE) /
 		ALIGN(MAX_MULTIPAGE_BUFFER_PACKET +
-		sizeof(struct vstor_packet) + sizeof(u64) -
-		vmscsi_size_delta,
+		sizeof(struct vstor_packet) + sizeof(u64),
 		sizeof(u64)));
 
 #if IS_ENABLED(CONFIG_SCSI_FC_ATTRS)
-- 
2.25.1

RE: [PATCH 1/3] scsi: storvsc: Fix max_outstanding_req_per_channel for Win8 and newer

[Dexuan Cui]

> From: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> Sent: Thursday, December 17, 2020 12:33 PM
> 
> Current code overestimates the value of max_outstanding_req_per_channel
> for Win8 and newer hosts, since vmscsi_size_delta is set to the initial
> value of sizeof(vmscsi_win8_extension) rather than zero.  This may lead
> to wrong decisions when using ring_avail_percent_lowater equals to zero.
> The estimate of max_outstanding_req_per_channel is 'exact' for Win7 and
> older hosts.  A better choice, keeping the algorithm for the estimation
> simple, is to err the other way around, i.e., to underestimate for Win7
> and older but to use the exact value for Win8 and newer.
> 
> Suggested-by: Dexuan Cui <decui@microsoft.com>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: linux-scsi@vger.kernel.org

Reviewed-by: Dexuan Cui <decui@microsoft.com>

RE: [PATCH 1/3] scsi: storvsc: Fix max_outstanding_req_per_channel for Win8 and newer

[Michael Kelley]

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Thursday, December 17, 2020 12:33 PM
> 
> Current code overestimates the value of max_outstanding_req_per_channel
> for Win8 and newer hosts, since vmscsi_size_delta is set to the initial
> value of sizeof(vmscsi_win8_extension) rather than zero.  This may lead
> to wrong decisions when using ring_avail_percent_lowater equals to zero.
> The estimate of max_outstanding_req_per_channel is 'exact' for Win7 and
> older hosts.  A better choice, keeping the algorithm for the estimation
> simple, is to err the other way around, i.e., to underestimate for Win7
> and older but to use the exact value for Win8 and newer.
> 
> Suggested-by: Dexuan Cui <decui@microsoft.com>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: linux-scsi@vger.kernel.org
> ---
>  drivers/scsi/storvsc_drv.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index ded00a89bfc4e..64298aa2f151e 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -2141,12 +2141,15 @@ static int __init storvsc_drv_init(void)
>  	 * than the ring buffer size since that page is reserved for
>  	 * the ring buffer indices) by the max request size (which is
>  	 * vmbus_channel_packet_multipage_buffer + struct vstor_packet + u64)
> +	 *
> +	 * The computation underestimates max_outstanding_req_per_channel
> +	 * for Win7 and older hosts because it does not take into account
> +	 * the vmscsi_size_delta correction to the max request size.
>  	 */
>  	max_outstanding_req_per_channel =
>  		((storvsc_ringbuffer_size - PAGE_SIZE) /
>  		ALIGN(MAX_MULTIPAGE_BUFFER_PACKET +
> -		sizeof(struct vstor_packet) + sizeof(u64) -
> -		vmscsi_size_delta,
> +		sizeof(struct vstor_packet) + sizeof(u64),
>  		sizeof(u64)));
> 
>  #if IS_ENABLED(CONFIG_SCSI_FC_ATTRS)
> --
> 2.25.1

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

[PATCH 2/3] scsi: storvsc: Resolve data race in storvsc_probe()

[Andrea Parri (Microsoft)]

vmscsi_size_delta can be written concurrently by multiple instances of
storvsc_probe(), corresponding to multiple synthetic IDE/SCSI devices;
cf. storvsc_drv's probe_type == PROBE_PREFER_ASYNCHRONOUS.  Change the
global variable vmscsi_size_delta to per-synthetic-IDE/SCSI-device.

Suggested-by: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/storvsc_drv.c | 45 +++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 64298aa2f151e..8714355cb63e7 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -216,18 +216,6 @@ struct vmscsi_request {
 
 } __attribute((packed));
 
-
-/*
- * The size of the vmscsi_request has changed in win8. The
- * additional size is because of new elements added to the
- * structure. These elements are valid only when we are talking
- * to a win8 host.
- * Track the correction to size we need to apply. This value
- * will likely change during protocol negotiation but it is
- * valid to start by assuming pre-Win8.
- */
-static int vmscsi_size_delta = sizeof(struct vmscsi_win8_extension);
-
 /*
  * The list of storage protocols in order of preference.
  */
@@ -449,6 +437,17 @@ struct storvsc_device {
 	unsigned char path_id;
 	unsigned char target_id;
 
+	/*
+	 * The size of the vmscsi_request has changed in win8. The
+	 * additional size is because of new elements added to the
+	 * structure. These elements are valid only when we are talking
+	 * to a win8 host.
+	 * Track the correction to size we need to apply. This value
+	 * will likely change during protocol negotiation but it is
+	 * valid to start by assuming pre-Win8.
+	 */
+	int vmscsi_size_delta;
+
 	/*
 	 * Max I/O, the device can support.
 	 */
@@ -762,7 +761,7 @@ static void  handle_multichannel_storage(struct hv_device *device, int max_chns)
 
 	ret = vmbus_sendpacket(device->channel, vstor_packet,
 			       (sizeof(struct vstor_packet) -
-			       vmscsi_size_delta),
+			       stor_device->vmscsi_size_delta),
 			       (unsigned long)request,
 			       VM_PKT_DATA_INBAND,
 			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
@@ -816,9 +815,14 @@ static int storvsc_execute_vstor_op(struct hv_device *device,
 				    struct storvsc_cmd_request *request,
 				    bool status_check)
 {
+	struct storvsc_device *stor_device;
 	struct vstor_packet *vstor_packet;
 	int ret, t;
 
+	stor_device = get_out_stor_device(device);
+	if (!stor_device)
+		return -ENODEV;
+
 	vstor_packet = &request->vstor_packet;
 
 	init_completion(&request->wait_event);
@@ -826,7 +830,7 @@ static int storvsc_execute_vstor_op(struct hv_device *device,
 
 	ret = vmbus_sendpacket(device->channel, vstor_packet,
 			       (sizeof(struct vstor_packet) -
-			       vmscsi_size_delta),
+			       stor_device->vmscsi_size_delta),
 			       (unsigned long)request,
 			       VM_PKT_DATA_INBAND,
 			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
@@ -903,7 +907,7 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
 			sense_buffer_size =
 				vmstor_protocols[i].sense_buffer_size;
 
-			vmscsi_size_delta =
+			stor_device->vmscsi_size_delta =
 				vmstor_protocols[i].vmscsi_size_delta;
 
 			break;
@@ -1249,7 +1253,7 @@ static void storvsc_on_channel_callback(void *context)
 		if (request == &stor_device->init_request ||
 		    request == &stor_device->reset_request) {
 			memcpy(&request->vstor_packet, packet,
-			       (sizeof(struct vstor_packet) - vmscsi_size_delta));
+			       (sizeof(struct vstor_packet) - stor_device->vmscsi_size_delta));
 			complete(&request->wait_event);
 		} else {
 			storvsc_on_receive(stor_device, packet, request);
@@ -1461,7 +1465,7 @@ static int storvsc_do_io(struct hv_device *device,
 	vstor_packet->flags |= REQUEST_COMPLETION_FLAG;
 
 	vstor_packet->vm_srb.length = (sizeof(struct vmscsi_request) -
-					vmscsi_size_delta);
+					stor_device->vmscsi_size_delta);
 
 
 	vstor_packet->vm_srb.sense_info_length = sense_buffer_size;
@@ -1478,12 +1482,12 @@ static int storvsc_do_io(struct hv_device *device,
 				request->payload, request->payload_sz,
 				vstor_packet,
 				(sizeof(struct vstor_packet) -
-				vmscsi_size_delta),
+				stor_device->vmscsi_size_delta),
 				(unsigned long)request);
 	} else {
 		ret = vmbus_sendpacket(outgoing_channel, vstor_packet,
 			       (sizeof(struct vstor_packet) -
-				vmscsi_size_delta),
+				stor_device->vmscsi_size_delta),
 			       (unsigned long)request,
 			       VM_PKT_DATA_INBAND,
 			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
@@ -1589,7 +1593,7 @@ static int storvsc_host_reset_handler(struct scsi_cmnd *scmnd)
 
 	ret = vmbus_sendpacket(device->channel, vstor_packet,
 			       (sizeof(struct vstor_packet) -
-				vmscsi_size_delta),
+				stor_device->vmscsi_size_delta),
 			       (unsigned long)&stor_device->reset_request,
 			       VM_PKT_DATA_INBAND,
 			       VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
@@ -1939,6 +1943,7 @@ static int storvsc_probe(struct hv_device *device,
 	init_waitqueue_head(&stor_device->waiting_to_drain);
 	stor_device->device = device;
 	stor_device->host = host;
+	stor_device->vmscsi_size_delta = sizeof(struct vmscsi_win8_extension);
 	spin_lock_init(&stor_device->lock);
 	hv_set_drvdata(device, stor_device);
 
-- 
2.25.1

RE: [PATCH 2/3] scsi: storvsc: Resolve data race in storvsc_probe()

[Dexuan Cui]

> From: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> Sent: Thursday, December 17, 2020 12:33 PM
> 
> vmscsi_size_delta can be written concurrently by multiple instances of
> storvsc_probe(), corresponding to multiple synthetic IDE/SCSI devices;
> cf. storvsc_drv's probe_type == PROBE_PREFER_ASYNCHRONOUS.  Change
> the
> global variable vmscsi_size_delta to per-synthetic-IDE/SCSI-device.
> 
> Suggested-by: Dexuan Cui <decui@microsoft.com>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: linux-scsi@vger.kernel.org

Reviewed-by: Dexuan Cui <decui@microsoft.com>

RE: [PATCH 2/3] scsi: storvsc: Resolve data race in storvsc_probe()

[Michael Kelley]

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Thursday, December 17, 2020 12:33 PM
> 
> vmscsi_size_delta can be written concurrently by multiple instances of
> storvsc_probe(), corresponding to multiple synthetic IDE/SCSI devices;
> cf. storvsc_drv's probe_type == PROBE_PREFER_ASYNCHRONOUS.  Change the
> global variable vmscsi_size_delta to per-synthetic-IDE/SCSI-device.
> 
> Suggested-by: Dexuan Cui <decui@microsoft.com>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: linux-scsi@vger.kernel.org
> ---
>  drivers/scsi/storvsc_drv.c | 45 +++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 20 deletions(-)
> 

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

[PATCH 3/3] scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback()

[Andrea Parri (Microsoft)]

Check that the packet is of the expected size at least, don't copy data
past the packet.

Reported-by: Saruhan Karademir <skarade@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
 drivers/scsi/storvsc_drv.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 8714355cb63e7..4b8bde2750fac 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1250,6 +1250,12 @@ static void storvsc_on_channel_callback(void *context)
 		request = (struct storvsc_cmd_request *)
 			((unsigned long)desc->trans_id);
 
+		if (hv_pkt_datalen(desc) < sizeof(struct vstor_packet) -
+				stor_device->vmscsi_size_delta) {
+			dev_err(&device->device, "Invalid packet len\n");
+			continue;
+		}
+
 		if (request == &stor_device->init_request ||
 		    request == &stor_device->reset_request) {
 			memcpy(&request->vstor_packet, packet,
-- 
2.25.1

RE: [PATCH 3/3] scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback()

[Dexuan Cui]

> From: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> Sent: Thursday, December 17, 2020 12:33 PM
> 
> Check that the packet is of the expected size at least, don't copy data
> past the packet.
> 
> Reported-by: Saruhan Karademir <skarade@microsoft.com>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: linux-scsi@vger.kernel.org

Reviewed-by: Dexuan Cui <decui@microsoft.com>

RE: [PATCH 3/3] scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback()

[Michael Kelley]

From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Thursday, December 17, 2020 12:33 PM
> 
> Check that the packet is of the expected size at least, don't copy data
> past the packet.
> 
> Reported-by: Saruhan Karademir <skarade@microsoft.com>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
> Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
> Cc: linux-scsi@vger.kernel.org
> ---
>  drivers/scsi/storvsc_drv.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 8714355cb63e7..4b8bde2750fac 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1250,6 +1250,12 @@ static void storvsc_on_channel_callback(void *context)
>  		request = (struct storvsc_cmd_request *)
>  			((unsigned long)desc->trans_id);
> 
> +		if (hv_pkt_datalen(desc) < sizeof(struct vstor_packet) -
> +				stor_device->vmscsi_size_delta) {
> +			dev_err(&device->device, "Invalid packet len\n");
> +			continue;
> +		}
> +
>  		if (request == &stor_device->init_request ||
>  		    request == &stor_device->reset_request) {
>  			memcpy(&request->vstor_packet, packet,
> --
> 2.25.1

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

Re: [PATCH 3/3] scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback()

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 598 bytes --]

On Thu, Dec 17, Andrea Parri (Microsoft) wrote:

> Check that the packet is of the expected size at least, don't copy data
> past the packet.

> +		if (hv_pkt_datalen(desc) < sizeof(struct vstor_packet) -
> +				stor_device->vmscsi_size_delta) {
> +			dev_err(&device->device, "Invalid packet len\n");
> +			continue;
> +		}
> +

Sorry for being late:

It might be just cosmetic, but should this check be done prior the call to vmbus_request_addr()?


Unrelated: my copy of vmbus_request_addr() can return 0, which is apparently not handled by this loop in storvsc_on_channel_callback().


Olaf




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback()

[Andrea Parri]

Hi Olaf,

On Mon, Mar 29, 2021 at 06:37:21PM +0200, Olaf Hering wrote:
> On Thu, Dec 17, Andrea Parri (Microsoft) wrote:
> 
> > Check that the packet is of the expected size at least, don't copy data
> > past the packet.
> 
> > +		if (hv_pkt_datalen(desc) < sizeof(struct vstor_packet) -
> > +				stor_device->vmscsi_size_delta) {
> > +			dev_err(&device->device, "Invalid packet len\n");
> > +			continue;
> > +		}
> > +
> 
> Sorry for being late:
> 
> It might be just cosmetic, but should this check be done prior the call to vmbus_request_addr()?

TBH, I'm not immediately seeing why it 'should'; it could make sense to move
the check on the packet data length.


> Unrelated: my copy of vmbus_request_addr() can return 0, which is apparently not handled by this loop in storvsc_on_channel_callback().

Indeed, IDs of 0 are reserved for so called unsolicited messages; I think we
should check that storvsc_on_io_completion() is not called on such messages.

Thanks,
  Andrea

Re: [PATCH 0/3] scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback() -- Take 2

[Martin K. Petersen]

Andrea,

> This series is to address the problems mentioned in:
>
>   4da3a54f5a0258 ("Revert "scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback()"")
>
> (cf., in particular, patch 2/3) and to re-introduce the validation in
> question (patch 3/3); patch 1/3 emerged from internal review of these
> two patches and is a related fix.

Applied to 5.12/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/3] scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback() -- Take 2

[Martin K. Petersen]

On Thu, 17 Dec 2020 21:33:18 +0100, Andrea Parri (Microsoft) wrote:

> This series is to address the problems mentioned in:
> 
>   4da3a54f5a0258 ("Revert "scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback()"")
> 
> (cf., in particular, patch 2/3) and to re-introduce the validation in
> question (patch 3/3); patch 1/3 emerged from internal review of these
> two patches and is a related fix.
> 
> [...]

Applied to 5.12/scsi-queue, thanks!

[1/3] scsi: storvsc: Fix max_outstanding_req_per_channel for Win8 and newer
      https://git.kernel.org/mkp/scsi/c/ab548fd21e1c
[2/3] scsi: storvsc: Resolve data race in storvsc_probe()
      https://git.kernel.org/mkp/scsi/c/244808e03029
[3/3] scsi: storvsc: Validate length of incoming packet in storvsc_on_channel_callback()
      https://git.kernel.org/mkp/scsi/c/91b1b640b834

-- 
Martin K. Petersen	Oracle Linux Engineering