cmnt: [trusty/linux xenial/linux artful/linux bionic/linux 1/1] floppy: Do not copy a kernel pointer to user memory in FDGETPRM ioctl

[Khaled Elmously]

On 2018-06-05 14:36:14 , Andy Whitcroft wrote:
> On Mon, Jun 04, 2018 at 05:35:05PM -0400, Khaled Elmously wrote:
> > On 2018-05-29 14:38:27 , Andy Whitcroft wrote:
> > > The final field of a floppy_struct is the field "name", which is a pointer
> > > to a string in kernel memory.  The kernel pointer should not be copied to
> > > user memory.  The FDGETPRM ioctl copies a floppy_struct to user memory,
> > > including this "name" field.  This pointer cannot be used by the user
> > > and it will leak a kernel address to user-space, which will reveal the
> > > location of kernel code and data and undermine KASLR protection.
> > > 
> > > Model this code after the compat ioctl which copies the returned data
> > > to a previously cleared temporary structure on the stack (excluding the
> > > name pointer) and copy out to userspace from there.  As we already have
> > > an inparam union with an appropriate member and that memory is already
> > > cleared even for read only calls make use of that as a temporary store.
> > > 
> > > Based on an initial patch by Brian Belleville.
> > > 
> > > CVE-2018-7755
> > > Signed-off-by: Andy Whitcroft <apw at canonical.com>
> > > ---
> > >  drivers/block/floppy.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> > > index eae484acfbbc..0a860603a5b6 100644
> > > --- a/drivers/block/floppy.c
> > > +++ b/drivers/block/floppy.c
> > > @@ -3470,6 +3470,8 @@ static int fd_locked_ioctl(struct block_device *bdev, fmode_t mode, unsigned int
> > >  					  (struct floppy_struct **)&outparam);
> > >  		if (ret)
> > >  			return ret;
> > > +		memcpy(&inparam.g, outparam, offsetof(struct floppy_struct, name));
> > > +		outparam = &inparam.g;
> > >  		break;
> > >  	case FDMSGON:
> > >  		UDP->flags |= FTD_MSG;
> > 
> > 
> > Not important, but I personally would have thought that just zeroing out the char * from outparam would be "cheaper" than copying everything _but_ the char pointer:
> > 
> > memset(outparam + offsetof(struct floppy_struct, name), 
> >        0, 
> >        sizeof(struct floppy_struct) - offsetof(struct floppy_struct, name));
> > 
> > 
> > Doesn't look like the savings would be significant though.
> 
> We can't do that because outparam as returned is pointing to the kernel
> internal store for the original data.  If we clear name in outparam we
> are clearing it for the kernel en-toto and would lose the name.

I see what you mean - thanks for explaining that :)

> 
> -apw