[T/X/A/B/C] CVE-2018-7755 -- floppy ioctl FDGETPRM exposes kernel pointer

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[T/X/A/B/C] CVE-2018-7755 -- floppy ioctl FDGETPRM exposes kernel pointer

Andy Whitcroft-3
CVE-2018-7755:
        An issue was discovered in the fd_locked_ioctl function in
        drivers/block/floppy.c in the Linux kernel through 4.15.7. The
        floppy driver will copy a kernel pointer to user memory in response
        to the FDGETPRM ioctl. An attacker can send the FDGETPRM ioctl and
        use the obtained kernel pointer to discover the location of kernel
        code and data and bypass kernel security protections such as KASLR.

Ensure this pointer is not populated in the data as returned to
userspace.  Proposing for SRU to trusty, xenial, artful, bionic, and
cosmic.

-apw

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

Andy Whitcroft-3
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 <[hidden email]>
---
 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;
--
2.17.0


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [T/X/A/B/C] CVE-2018-7755 -- floppy ioctl FDGETPRM exposes kernel pointer

Andy Whitcroft-3
In reply to this post by Andy Whitcroft-3
On Tue, May 29, 2018 at 02:38:26PM +0100, Andy Whitcroft wrote:

> CVE-2018-7755:
> An issue was discovered in the fd_locked_ioctl function in
> drivers/block/floppy.c in the Linux kernel through 4.15.7. The
> floppy driver will copy a kernel pointer to user memory in response
> to the FDGETPRM ioctl. An attacker can send the FDGETPRM ioctl and
> use the obtained kernel pointer to discover the location of kernel
> code and data and bypass kernel security protections such as KASLR.
>
> Ensure this pointer is not populated in the data as returned to
> userspace.  Proposing for SRU to trusty, xenial, artful, bionic, and
> cosmic.

Note that this should be applied as UBUNTU: SAUCE: as it has not yet
made it upstream.

-apw

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

Thadeu Lima de Souza Cascardo-3
In reply to this post by Andy Whitcroft-3
On Tue, May 29, 2018 at 02:38:27PM +0100, 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 <[hidden email]>
> ---
>  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;
> --
> 2.17.0
>

Code seems fine.

Acked-by: Thadeu Lima de Souza Cascardo <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

Kleber Souza
In reply to this post by Andy Whitcroft-3
On 05/29/18 06:38, 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 <[hidden email]>
> ---
>  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;
>

Looks good.

Acked-by: Kleber Sacilotto de Souza <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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
In reply to this post by Andy Whitcroft-3
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 <[hidden email]>
> ---
>  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.

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED/T: [T/X/A/B/C] CVE-2018-7755 -- floppy ioctl FDGETPRM exposes kernel pointer

Juerg Haefliger
In reply to this post by Andy Whitcroft-3
Applied to trusty/master-next.

...Juerg

On 05/29/2018 03:38 PM, Andy Whitcroft wrote:

> CVE-2018-7755:
> An issue was discovered in the fd_locked_ioctl function in
> drivers/block/floppy.c in the Linux kernel through 4.15.7. The
> floppy driver will copy a kernel pointer to user memory in response
> to the FDGETPRM ioctl. An attacker can send the FDGETPRM ioctl and
> use the obtained kernel pointer to discover the location of kernel
> code and data and bypass kernel security protections such as KASLR.
>
> Ensure this pointer is not populated in the data as returned to
> userspace.  Proposing for SRU to trusty, xenial, artful, bionic, and
> cosmic.
>
> -apw
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

APPLIED/X: [T/X/A/B/C] CVE-2018-7755 -- floppy ioctl FDGETPRM exposes kernel pointer

Juerg Haefliger
In reply to this post by Andy Whitcroft-3
Applied to xenial/master-next.

...Juerg

On 05/29/2018 03:38 PM, Andy Whitcroft wrote:

> CVE-2018-7755:
> An issue was discovered in the fd_locked_ioctl function in
> drivers/block/floppy.c in the Linux kernel through 4.15.7. The
> floppy driver will copy a kernel pointer to user memory in response
> to the FDGETPRM ioctl. An attacker can send the FDGETPRM ioctl and
> use the obtained kernel pointer to discover the location of kernel
> code and data and bypass kernel security protections such as KASLR.
>
> Ensure this pointer is not populated in the data as returned to
> userspace.  Proposing for SRU to trusty, xenial, artful, bionic, and
> cosmic.
>
> -apw
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Andy Whitcroft-3
In reply to this post by Khaled Elmously
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 <[hidden email]>
> > ---
> >  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.

-apw

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

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

Juerg Haefliger
In reply to this post by Andy Whitcroft-3
And applied to artful, bionic and cosmic master-next.

...Juerg

On 05/29/2018 03:38 PM, 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 <[hidden email]>
> ---
>  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;
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: 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
In reply to this post by Andy Whitcroft-3
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 <[hidden email]>
> > > ---
> > >  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

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team