linux-kernel - Re: [RFC][PATCHES] drivers/scsi/sg.c uaccess cleanups/fixes

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]

Date:   Thu, 17 Oct 2019 23:44:28 +0200
From:   Douglas Gilbert <dgilbert@...erlog.com>
To:     Al Viro <viro@...iv.linux.org.uk>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCHES] drivers/scsi/sg.c uaccess cleanups/fixes

On 2019-10-17 9:36 p.m., Al Viro wrote:
> On Wed, Oct 16, 2019 at 09:25:40PM +0100, Al Viro wrote:
> 
>> FWIW, callers of __copy_from_user() remaining in the generic code:
> 
>> 6) drivers/scsi/sg.c nest: sg_read() ones are memdup_user() in disguise
>> (i.e. fold with immediately preceding kmalloc()s).  sg_new_write() -
>> fold with access_ok() into copy_from_user() (for both call sites).
>> sg_write() - lose access_ok(), use copy_from_user() (both call sites)
>> and get_user() (instead of the solitary __get_user() there).
> 
> Turns out that there'd been outright redundant access_ok() calls (not
> even warranted by __copy_...) *and* several __put_user()/__get_user()
> with no checking of return value (access_ok() was there, handling of
> unmapped addresses wasn't).  The latter go back at least to 2.1.early...
> 
> I've got a series that presumably fixes and cleans the things up
> in that area; it didn't get any serious testing (the kernel builds
> and boots, smartctl works as well as it used to, but that's not
> worth much - all it says is that SG_IO doesn't fail terribly;
> I don't have any test setup for really working with /dev/sg*).
> 
> IOW, it needs more review and testing - this is _not_ a pull request.
> It's in vfs.git#work.sg; individual patches are in followups.
> Shortlog/diffstat:
> Al Viro (8):
>        sg_ioctl(): fix copyout handling
>        sg_new_write(): replace access_ok() + __copy_from_user() with copy_from_user()
>        sg_write(): __get_user() can fail...
>        sg_read(): simplify reading ->pack_id of userland sg_io_hdr_t
>        sg_new_write(): don't bother with access_ok
>        sg_read(): get rid of access_ok()/__copy_..._user()
>        sg_write(): get rid of access_ok()/__copy_from_user()/__get_user()
>        SG_IO: get rid of access_ok()
> 
>   drivers/scsi/sg.c | 98 ++++++++++++++++++++++++++++++++----------------------------------------------------------------
>   1 file changed, 32 insertions(+), 66 deletions(-)

Al,
I am aware of these and have a 23 part patchset on the linux-scsi list
for review (see https://marc.info/?l=linux-scsi&m=157052102631490&w=2 )
that amongst other things fixes all of these. It also re-adds the
functionality removed from the bsg driver last year. Unfortunately that
review process is going very slowly, so I have no objections if you
apply these now.

It is unlikely that these changes will introduce any bugs (they didn't in
my testing). If you want to do more testing you may find the sg3_utils
package helpful, especially in the testing directory:
     https://github.com/hreinecke/sg3_utils

Doug Gilbert