On Mon, Aug 27, 2018 at 12:45:25AM -0700, Kees Cook wrote: > On Tue, May 29, 2018 at 6:27 AM, Andy Whitcroft <robobotbot...@gmail.com> > wrote: > > On Wed, Mar 07, 2018 at 04:02:45PM -0800, Brian Belleville 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 the "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. Instead, copy the floppy_struct except for the "name" > >> field. > >> > >> Signed-off-by: Brian Belleville <bbell...@uci.edu> > >> --- > >> drivers/block/floppy.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c > >> index eae484a..4d4a422 100644 > >> --- a/drivers/block/floppy.c > >> +++ b/drivers/block/floppy.c > >> @@ -3470,6 +3470,7 @@ static int fd_locked_ioctl(struct block_device > >> *bdev, fmode_t mode, unsigned int > >> (struct floppy_struct > >> **)&outparam); > >> if (ret) > >> return ret; > >> + size = offsetof(struct floppy_struct, name); > >> break; > >> case FDMSGON: > >> UDP->flags |= FTD_MSG; > > > > I am not sure it is reasonable to simply set size here to the length of the > > valid data. Though in the real world everyonne should be using the defines > > and those should include the full length, the code itself does not require > > this, it only prevents overly long reads. So I think it is possible to do > > this read with a shorter userspace buffer; with this change we would > > then write beyond the end of the buffer. > > > > This also seems to introduce a slight behavioural difference between the > > primary and compat calls. The compat call already elides the name but it > > also is copying into a new structure for return and this is pre-cleared, > > so the name will always be null for the compat case and undefined for > > the primary ioctl. > > > > Perhaps the below patch would be more appropriate. > > > > -apw > > > > From ddb8c77229a9507fa5575c910d2847e123a9c94c Mon Sep 17 00:00:00 2001 > > From: Andy Whitcroft <a...@canonical.com> > > Date: Tue, 29 May 2018 13:04:15 +0100 > > Subject: [PATCH 1/1] floppy: Do not copy a kernel pointer to user memory in > > FDGETPRM ioctl > > > > 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 <a...@canonical.com> > > I didn't see this land anywhere? Who's tree is this going through? > > -Kees >
Looks like a pretty simple fix, but still don't see this one anywhere... maybe Jiri or Jens could pick it up (as per get_maintainer output :) -- Joe > > --- > > drivers/block/floppy.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c > > index 8ec7235fc93b..7512f6ff7c43 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; > > -- > > 2.17.0 > > > > > > -- > Kees Cook > Pixel Security
