* [PATCH] bsg: allocate response buffer if requested
@ 2019-01-30 9:44 Hannes Reinecke
2019-01-30 16:35 ` Bart Van Assche
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Hannes Reinecke @ 2019-01-30 9:44 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-block, Hannes Reinecke, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] bsg: allocate response buffer if requested
2019-01-30 9:44 [PATCH] bsg: allocate response buffer if requested Hannes Reinecke
@ 2019-01-30 16:35 ` Bart Van Assche
2019-02-04 0:11 ` Bart Van Assche
2019-02-04 14:40 ` Christoph Hellwig
2 siblings, 0 replies; 6+ messages in thread
From: Bart Van Assche @ 2019-01-30 16:35 UTC (permalink / raw)
To: Hannes Reinecke, Jens Axboe; +Cc: linux-block, Hannes Reinecke
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>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] bsg: allocate response buffer if requested
2019-01-30 9:44 [PATCH] bsg: allocate response buffer if requested Hannes Reinecke
2019-01-30 16:35 ` Bart Van Assche
@ 2019-02-04 0:11 ` Bart Van Assche
2019-02-04 14:40 ` Christoph Hellwig
2 siblings, 0 replies; 6+ messages in thread
From: Bart Van Assche @ 2019-02-04 0:11 UTC (permalink / raw)
To: Hannes Reinecke, Jens Axboe; +Cc: linux-block, Hannes Reinecke
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>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] bsg: allocate response buffer if requested
2019-01-30 9:44 [PATCH] bsg: allocate response buffer if requested Hannes Reinecke
2019-01-30 16:35 ` Bart Van Assche
2019-02-04 0:11 ` Bart Van Assche
@ 2019-02-04 14:40 ` Christoph Hellwig
2019-02-04 15:37 ` Hannes Reinecke
2 siblings, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2019-02-04 14:40 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Jens Axboe, linux-block, Hannes Reinecke
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] bsg: allocate response buffer if requested
2019-02-04 14:40 ` Christoph Hellwig
@ 2019-02-04 15:37 ` Hannes Reinecke
2019-02-04 15:43 ` Christoph Hellwig
0 siblings, 1 reply; 6+ messages in thread
From: Hannes Reinecke @ 2019-02-04 15:37 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Jens Axboe, linux-block, 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)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] bsg: allocate response buffer if requested
2019-02-04 15:37 ` Hannes Reinecke
@ 2019-02-04 15:43 ` Christoph Hellwig
0 siblings, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2019-02-04 15:43 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Christoph Hellwig, Jens Axboe, linux-block, Hannes Reinecke
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-02-04 15:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-01-30 9:44 [PATCH] bsg: allocate response buffer if requested Hannes Reinecke
2019-01-30 16:35 ` Bart Van Assche
2019-02-04 0:11 ` Bart Van Assche
2019-02-04 14:40 ` Christoph Hellwig
2019-02-04 15:37 ` Hannes Reinecke
2019-02-04 15:43 ` Christoph Hellwig
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).