Re: [PATCH 3.16 42/61] scsi: sg: don't return bogus Sg_requests

[Ben Hutchings <ben@decadent.org.uk>]

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

On Tue, 2020-06-09 at 14:28 -0400, Tony Battersby wrote:
> On 6/9/20 2:04 PM, Ben Hutchings wrote:
> > 3.16.85-rc1 review patch.  If anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: Johannes Thumshirn <jthumshirn@suse.de>
> > 
> > commit 48ae8484e9fc324b4968d33c585e54bc98e44d61 upstream.
> > 
> > If the list search in sg_get_rq_mark() fails to find a valid request, we
> > return a bogus element. This then can later lead to a GPF in
> > sg_remove_scat().
> > 
> > So don't return bogus Sg_requests in sg_get_rq_mark() but NULL in case
> > the list search doesn't find a valid request.
> > 
> > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
> > Reported-by: Andrey Konovalov <andreyknvl@google.com>
> > Cc: Hannes Reinecke <hare@suse.de>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: Doug Gilbert <dgilbert@interlog.com>
> > Reviewed-by: Hannes Reinecke <hare@suse.de>
> > Acked-by: Doug Gilbert <dgilbert@interlog.com>
> > Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> > Cc: Tony Battersby <tonyb@cybernetics.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > ---
> >  drivers/scsi/sg.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > --- a/drivers/scsi/sg.c
> > +++ b/drivers/scsi/sg.c
> > @@ -2085,11 +2085,12 @@ sg_get_rq_mark(Sg_fd * sfp, int pack_id)
> >  		if ((1 == resp->done) && (!resp->sg_io_owned) &&
> >  		    ((-1 == pack_id) || (resp->header.pack_id == pack_id))) {
> >  			resp->done = 2;	/* guard against other readers */
> > -			break;
> > +			write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
> > +			return resp;
> >  		}
> >  	}
> >  	write_unlock_irqrestore(&sfp->rq_list_lock, iflags);
> > -	return resp;
> > +	return NULL;
> >  }
> >  
> >  /* always adds to end of list */
> > 
> The following "cleanup" commit to the sg driver introduced a number of bugs:
> 
> 109bade9c625 ("scsi: sg: use standard lists for sg_requests") (v4.12-rc1)
> 
> This one bad commit requires all of the following fixes:
> 
> 48ae8484e9fc ("scsi: sg: don't return bogus Sg_requests") (v4.12-rc1)
> bd46fc406b30 ("scsi: sg: off by one in sg_ioctl()") (v4.13-rc7)
> 4759df905a47 ("scsi: sg: factor out sg_fill_request_table()") (v4.14-rc1)
> 3e0097499839 ("scsi: sg: fixup infoleak when using SG_GET_REQUEST_TABLE") (v4.14-rc1)
> 587c3c9f286c ("scsi: sg: Re-fix off by one in sg_fill_request_table()") (v4.14-rc6)
> 
> AFAIK, there is no reason to backport any of these changes to -stable,
> but if for some reason you do need to backport any one of these patches,
> then make sure you get all of them.

I couldn't see how to backport some of the more recent fixes without
applying this change first.  For this review cycle I've picked all of
the bug fixes that were on the 4.4-stable and 4.9-stable branches and
not yet in 3.16-stable, so I do have all the fixes you identified
above.

Ben.

> My guess is that the initial buggy patch was backported to other -stable
> trees because the fixes for it looked important, and of course the fixes
> depended on the patch that introduced all of the problems to begin with.

-- 
Ben Hutchings
The two most common things in the universe are hydrogen and stupidity.



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]