| Message ID | 20190130094424.94255-1-hare@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bsg: allocate response buffer if requested | expand |
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>
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>
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?
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
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.
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 = {
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(-)