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

[Tony Battersby]

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@xxxxxxx>
>
> 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@xxxxxxx>
> Reported-by: Andrey Konovalov <andreyknvl@xxxxxxxxxx>
> Cc: Hannes Reinecke <hare@xxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Doug Gilbert <dgilbert@xxxxxxxxxxxx>
> Reviewed-by: Hannes Reinecke <hare@xxxxxxx>
> Acked-by: Doug Gilbert <dgilbert@xxxxxxxxxxxx>
> Signed-off-by: Martin K. Petersen <martin.petersen@xxxxxxxxxx>
> Cc: Tony Battersby <tonyb@xxxxxxxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
> ---
>  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.

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.

Tony Battersby
Cybernetics