linux-kernel - Re: [PATCH 4.12 004/106] scsi: sg: fix SG_DXFER_FROM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cd75de84-a0a5-f11e-baf0-16d99d4fdc14@googlemail.com>
Date:   Thu, 10 Aug 2017 09:09:34 +0100
From:   Chris Clayton <chris2553@...glemail.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org
Cc:     stable@...r.kernel.org, Johannes Thumshirn <jthumshirn@...e.de>,
        Douglas Gilbert <dgilbert@...erlog.com>,
        Hannes Reinecke <hare@...e.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>
Subject: Re: [PATCH 4.12 004/106] scsi: sg: fix SG_DXFER_FROM_DEV transfers



On 09/08/17 17:51, Greg Kroah-Hartman wrote:
> 4.12-stable review patch.  If anyone has any objections, please let me know.
> 
> -------
 I repeat my comments when the patch was queued for stable:

1. Johannes' commit message says that the transfer must have a length bigger than 0, so the code should return false if
the length is less than or equal to 0, but the test is for less than 0.

2. But in any case, there's another patch that removes all this sg_is_valid_dxfer() jiggery-pokery and replaces it with
a simpler test. It hasn't reached Linus' tree yet but is, I believe, cc'd to stable.


As Johannes said in response to the second of my comments, the patch that replaces sg_is_valid_dxfer() with a simpler
test is now in Linus' tree - commit f930c7043663188429cd9b254e9d761edfc101ce. Without that change, I think there is
still some breakage in sg.

Chris
-----------
> 
> From: Johannes Thumshirn <jthumshirn@...e.de>
> 
> commit 68c59fcea1f2c6a54c62aa896cc623c1b5bc9b47 upstream.
> 
> SG_DXFER_FROM_DEV transfers do not necessarily have a dxferp as we set
> it to NULL for the old sg_io read/write interface, but must have a
> length bigger than 0. This fixes a regression introduced by commit
> 28676d869bbb ("scsi: sg: check for valid direction before starting the
> request")
> 
> Signed-off-by: Johannes Thumshirn <jthumshirn@...e.de>
> Fixes: 28676d869bbb ("scsi: sg: check for valid direction before starting the request")
> Reported-by: Chris Clayton <chris2553@...glemail.com>
> Tested-by: Chris Clayton <chris2553@...glemail.com>
> Cc: Douglas Gilbert <dgilbert@...erlog.com>
> Reviewed-by: Hannes Reinecke <hare@...e.com>
> Tested-by: Chris Clayton <chris2553@...glemail.com>
> Acked-by: Douglas Gilbert <dgilbert@...erlog.com>
> Signed-off-by: Martin K. Petersen <martin.petersen@...cle.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> 
> ---
>  drivers/scsi/sg.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -758,8 +758,11 @@ static bool sg_is_valid_dxfer(sg_io_hdr_
>  		if (hp->dxferp || hp->dxfer_len > 0)
>  			return false;
>  		return true;
> -	case SG_DXFER_TO_DEV:
>  	case SG_DXFER_FROM_DEV:
> +		if (hp->dxfer_len < 0)
> +			return false;
> +		return true;
> +	case SG_DXFER_TO_DEV:
>  	case SG_DXFER_TO_FROM_DEV:
>  		if (!hp->dxferp || hp->dxfer_len == 0)
>  			return false;
> 
>