Re: Fw: data disclosure in ioctl sg inquiry

[FUJITA Tomonori <tomof@acm.org>]

On Mon, 03 Sep 2007 13:00:43 -0400
Douglas Gilbert <dougg@torque.net> wrote:

> FUJITA Tomonori wrote:
> > On Sun, 2 Sep 2007 04:56:01 -0700
> > Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> >>
> >> Begin forwarded message:
> >>
> >> Date: Mon, 27 Aug 2007 15:01:33 +0100
> >> From: Luciano Rocha <strange@nsk.no-ip.org>
> >> To: linux-kernel@vger.kernel.org
> >> Subject: data disclosure in ioctl sg inquiry
> >>
> >>
> >>
> >> (Please keep me CC'ed. Thanks.)
> >>
> >> Hello,
> >>
> >> While testing the SG INQUIRY command to a locked hard drive, connected
> >> with USB, I noted that the command result included garbage that seemed
> >> part of some other's process memory. Like bash functions, command
> >> arguments, etc..
> >>
> >> I make sure to memset the buffers before running the ioctl, so this seem
> >> to be data leaked from the kernel.
> >>
> >> Most of the code is verbatim from the example in the SCSI Generic HOWTO
> >> (<http://tldp.org/HOWTO/SCSI-Generic-HOWTO/pexample.html>).
> >>
> >> I include the code I used and sample output with data from running
> >> processes (or files?).
> >>
> >> I can't reproduce this on a firewire connected HDD, but I can with
> >> another USB connecte one (not locked).
> > 
> > $ ./keytool /dev/sdb
> > Some of the INQUIRY command's response:
> > 00 00 00 00 1f 00 00 00 4d 41 58 54 4f 52 20 53  ........MAXTOR S
> > 54 4d 33 32 35 30 38 32 30 41 20 20 20 20 20 20  TM3250820A      
> > 33 2e 41 41 11 00 00 00 23 31 31 38 38 32 32 32  3.AA....#1188222
> > 33 34 30 00 11 00 00 00 48 00 12 08 28 00 12 08  340.....H...(...
> > 00 00 00 00 59 00 00 00 64 69 66 66 20 2d 75 72  ....Y...diff -ur
> > 20 2d 2d 65 78 63 6c 75 64 65 20 2e 73 76 6e 20   --exclude .svn 
> > INQUIRY duration=3 millisecs, resid=60
> > 
> > Note that resid is 60. So, in your case, only the first 36 bytes are
> > valid. But I guess that it's not good to leak random kernel data to
> > user-space.
> > 
> > Can you try this patch?
> > 
> > ---
> >>From 2529dbda52ac2302eab9838910d59e13dedeb3bd Mon Sep 17 00:00:00 2001
> > From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Date: Sun, 2 Sep 2007 13:32:33 +0100
> > Subject: [PATCH] bio_copy_user use zeroed pages
> > 
> > bio_uncopy_user copies garbage to user-space buffer when the actual
> > transferred length is shorter than dxfer_len.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > ---
> >  fs/bio.c |    7 ++++++-
> >  1 files changed, 6 insertions(+), 1 deletions(-)
> > 
> > diff --git a/fs/bio.c b/fs/bio.c
> > index 29a44c1..26a7669 100644
> > --- a/fs/bio.c
> > +++ b/fs/bio.c
> > @@ -550,11 +550,16 @@ struct bio *bio_copy_user(struct request_queue *q, unsigned long uaddr,
> >  	ret = 0;
> >  	while (len) {
> >  		unsigned int bytes = PAGE_SIZE;
> > +		gfp_t mask;
> >  
> >  		if (bytes > len)
> >  			bytes = len;
> >  
> > -		page = alloc_page(q->bounce_gfp | GFP_KERNEL);
> > +		mask = q->bounce_gfp | GFP_KERNEL;
> > +		if (write_to_vm)
> > +			mask |= __GFP_ZERO;
> > +
> > +		page = alloc_page(mask);
> >  		if (!page) {
> >  			ret = -ENOMEM;
> >  			break;
> 
> Hello folks. This has been known about (or variations of
> it) for some time. The design approach has been:
>  - if the uid of the app is 0 (i.e. root) then we take
>    the fast approach (i.e. don't zero intermediate buffers)
>    as root can see the whole of ram anyway
>  - if the uid of the app is !=0 (i.e. a non-root user) then
>    zero intermediate buffers (and slow things down a bit)

It's tree for your sg, but I think that he uses block sg code and it
doesn't do such thing.