[PATCH] floppy: Do not copy a kernel pointer to user memory in FDGETPRM ioctl

[PATCH] floppy: Do not copy a kernel pointer to user memory in FDGETPRM ioctl

[Brian Belleville]

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 <bbellevi@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;
-- 
2.7.4

Re: [PATCH] floppy: Do not copy a kernel pointer to user memory in FDGETPRM ioctl

[Brian Belleville]

Hi, are there any comments on this patch or the issue I described? I 
have tested the FDGETPRM ioctl and confirmed that the struct it returns 
does contain a pointer to kernel data. I also have tested my patch, and 
with it applied the returned struct no longer contains a kernel pointer, 
but all other fields are still present.

Thank you,
Brian Belleville


On 03/07/2018 04:02 PM, 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 <bbellevi@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;
> 

Re: [PATCH] floppy: Do not copy a kernel pointer to user memory in FDGETPRM ioctl

[Andy Whitcroft]

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 <bbellevi@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 <apw@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 <apw@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 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

Re: [PATCH] floppy: Do not copy a kernel pointer to user memory in FDGETPRM ioctl

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2044 bytes --]

On Tue, 2018-05-29 at 14:27 +0100, Andy Whitcroft wrote:
[...]
> >From ddb8c77229a9507fa5575c910d2847e123a9c94c Mon Sep 17 00:00:00 2001
> From: Andy Whitcroft <apw@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 <apw@canonical.com>

Reviewed-and-tested-by: Ben Hutchings <ben@decadent.org.uk>

Ben.

> ---
>  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;
-- 
Ben Hutchings
If you seem to know what you are doing, you'll be given more to do.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] floppy: Do not copy a kernel pointer to user memory in FDGETPRM ioctl

[Kees Cook]

On Tue, May 29, 2018 at 6:27 AM, Andy Whitcroft <robobotbotbot@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 <bbellevi@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 <apw@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 <apw@canonical.com>

I didn't see this land anywhere? Who's tree is this going through?

-Kees

> ---
>  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

Re: [PATCH] floppy: Do not copy a kernel pointer to user memory in FDGETPRM ioctl

[Joe Lawrence]

On Mon, Aug 27, 2018 at 12:45:25AM -0700, Kees Cook wrote:
> On Tue, May 29, 2018 at 6:27 AM, Andy Whitcroft <robobotbotbot@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 <bbellevi@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 <apw@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 <apw@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

Re: [PATCH] floppy: Do not copy a kernel pointer to user memory in FDGETPRM ioctl

[Jens Axboe]

On 9/20/18 8:01 AM, Joe Lawrence wrote:
> On Mon, Aug 27, 2018 at 12:45:25AM -0700, Kees Cook wrote:
>> On Tue, May 29, 2018 at 6:27 AM, Andy Whitcroft <robobotbotbot@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 <bbellevi@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 <apw@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 <apw@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 :)

Applied for 4.19.

-- 
Jens Axboe