[PATCH] bsg: allocate response buffer if requested

[PATCH] bsg: allocate response buffer if requested

[Hannes Reinecke]

The 'response' buffer from bsg is mapped onto the SCSI sense buffer,
however after commit 82ed4db499b8 we need to allocate them ourselves
as the bsg queue is _not_ a SCSI queue, and hence the sense buffer
won't be allocated from the scsi stack.

Fixes: 82ed4db499b8 ("block: split scsi_request out of struct request")

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 block/bsg.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/block/bsg.c b/block/bsg.c
index 50e5f8f666f2..7554901096c8 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -81,6 +81,13 @@ static int bsg_scsi_fill_hdr(struct request *rq, struct sg_io_v4 *hdr,
 			return -ENOMEM;
 	}
 
+	if (hdr->response) {
+		sreq->sense = kzalloc(hdr->max_response_len, GFP_KERNEL);
+		if (!sreq->sense)
+			return -ENOMEM;
+	} else
+		sreq->sense = NULL;
+
 	if (copy_from_user(sreq->cmd, uptr64(hdr->request), sreq->cmd_len))
 		return -EFAULT;
 	if (blk_verify_command(sreq->cmd, mode))
@@ -128,7 +135,10 @@ static int bsg_scsi_complete_rq(struct request *rq, struct sg_io_v4 *hdr)
 
 static void bsg_scsi_free_rq(struct request *rq)
 {
-	scsi_req_free_cmd(scsi_req(rq));
+	struct scsi_request *sreq = scsi_req(rq);
+
+	kfree(sreq->sense);
+	scsi_req_free_cmd(sreq);
 }
 
 static const struct bsg_ops bsg_scsi_ops = {
-- 
2.16.4

Re: [PATCH] bsg: allocate response buffer if requested

[Bart Van Assche]

On Wed, 2019-01-30 at 10:44 +0100, Hannes Reinecke wrote:
> The 'response' buffer from bsg is mapped onto the SCSI sense buffer,
> however after commit 82ed4db499b8 we need to allocate them ourselves
> as the bsg queue is _not_ a SCSI queue, and hence the sense buffer
> won't be allocated from the scsi stack.
> 
> Fixes: 82ed4db499b8 ("block: split scsi_request out of struct request")
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>

Please add a "Cc: stable" tag. Anyway:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH] bsg: allocate response buffer if requested

[Bart Van Assche]

On 1/30/19 1:44 AM, Hannes Reinecke wrote:
> The 'response' buffer from bsg is mapped onto the SCSI sense buffer,
> however after commit 82ed4db499b8 we need to allocate them ourselves
> as the bsg queue is _not_ a SCSI queue, and hence the sense buffer
> won't be allocated from the scsi stack.
> 
> Fixes: 82ed4db499b8 ("block: split scsi_request out of struct request")

Since this patch fixes a bug introduced in kernel v4.11, should this 
patch have a "Cc: stable" tag? Anyway:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH] bsg: allocate response buffer if requested

[Christoph Hellwig]

On Wed, Jan 30, 2019 at 10:44:24AM +0100, Hannes Reinecke wrote:
> The 'response' buffer from bsg is mapped onto the SCSI sense buffer,
> however after commit 82ed4db499b8 we need to allocate them ourselves
> as the bsg queue is _not_ a SCSI queue, and hence the sense buffer
> won't be allocated from the scsi stack.

I don't think this is the full story.  Plain old bsg nodes are on SCSI
(or legacy IDE) request queues, so this should be initialized, and
your patch creates a memory leak by overwriting the sense pointer.

bsg-lib nodes aren't on scsi request queues, but they don't use the code
path your patch to start with.

What exactly is the reproducer for this problem? 

Re: [PATCH] bsg: allocate response buffer if requested

[Hannes Reinecke]

On 2/4/19 3:40 PM, Christoph Hellwig wrote:
> On Wed, Jan 30, 2019 at 10:44:24AM +0100, Hannes Reinecke wrote:
>> The 'response' buffer from bsg is mapped onto the SCSI sense buffer,
>> however after commit 82ed4db499b8 we need to allocate them ourselves
>> as the bsg queue is _not_ a SCSI queue, and hence the sense buffer
>> won't be allocated from the scsi stack.
> 
> I don't think this is the full story.  Plain old bsg nodes are on SCSI
> (or legacy IDE) request queues, so this should be initialized, and
> your patch creates a memory leak by overwriting the sense pointer.
> 
> bsg-lib nodes aren't on scsi request queues, but they don't use the code
> path your patch to start with.
> 
> What exactly is the reproducer for this problem?
> 
The problem is here:

static int bsg_scsi_complete_rq(struct request *rq, struct sg_io_v4 *hdr)
{
	struct scsi_request *sreq = scsi_req(rq);
	int ret = 0;

	/*
	 * fill in all the output members
	 */
	hdr->device_status = sreq->result & 0xff;
	hdr->transport_status = host_byte(sreq->result);
	hdr->driver_status = driver_byte(sreq->result);
	hdr->info = 0;
	if (hdr->device_status || hdr->transport_status || hdr->driver_status)
		hdr->info |= SG_INFO_CHECK;
	hdr->response_len = 0;

->	if (sreq->sense_len && hdr->response) {
		int len = min_t(unsigned int, hdr->max_response_len,
					sreq->sense_len);

		if (copy_to_user(uptr64(hdr->response), sreq->sense, len))
			ret = -EFAULT;
		else
			hdr->response_len = len;
	}

This expects the 'response' to be allocated.
Yet nowhere in the block/bsg.c we actually _do_ allocate the 'response' 
field.
And as the header is pretty much copied from userspace we don't really 
have any control about the contents of the 'response' nor the 
'response_len' parameter.

These fields used to be filled by mpt3sas (to hold the sense code), but 
with commit 651a01364994 ("scsi: scsi_transport_sas: switch to bsg-lib 
for SMP passthrough") the sense code handling got removed.

Alternatively we might kill 'response' handling altogether, but then 
that might have an impact on userland.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH] bsg: allocate response buffer if requested

[Christoph Hellwig]

On Mon, Feb 04, 2019 at 04:37:46PM +0100, Hannes Reinecke wrote:
> static int bsg_scsi_complete_rq(struct request *rq, struct sg_io_v4 *hdr)

So this is bsg_scsi_ops that you quote.

> This expects the 'response' to be allocated.
> Yet nowhere in the block/bsg.c we actually _do_ allocate the 'response'
> field.
> And as the header is pretty much copied from userspace we don't really have
> any control about the contents of the 'response' nor the 'response_len'
> parameter.
> 
> These fields used to be filled by mpt3sas (to hold the sense code), but with
> commit 651a01364994 ("scsi: scsi_transport_sas: switch to bsg-lib for SMP
> passthrough") the sense code handling got removed.

But all the transport bsg users actually use bsg_transport_ops and thus
should never end up in the above code.

Something in this bug report does not add up.