All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] USB: serial: Fix heap overflow in WHITEHEAT_GET_DTR_RTS
@ 2022-04-19  4:17 Kees Cook
  2022-04-20  7:54 ` Johan Hovold
  0 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2022-04-19  4:17 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Kees Cook, kernel test robot, Greg Kroah-Hartman, linux-usb,
	stable, linux-kernel, linux-hardening

This looks like it's harmless, as both the source and the destinations are
currently the same allocation size (4 bytes) and don't use their padding,
but if anything were to ever be added after the "mcr" member in "struct
whiteheat_private", it would be overwritten. The structs both have a
single u8 "mcr" member, but are 4 bytes in padded size. The memcpy()
destination was explicitly targeting the u8 member (size 1) with the
length of the whole structure (size 4), triggering the memcpy buffer
overflow warning:

In file included from include/linux/string.h:253,
                 from include/linux/bitmap.h:11,
                 from include/linux/cpumask.h:12,
                 from include/linux/smp.h:13,
                 from include/linux/lockdep.h:14,
                 from include/linux/spinlock.h:62,
                 from include/linux/mmzone.h:8,
                 from include/linux/gfp.h:6,
                 from include/linux/slab.h:15,
                 from drivers/usb/serial/whiteheat.c:17:
In function 'fortify_memcpy_chk',
    inlined from 'firm_send_command' at drivers/usb/serial/whiteheat.c:587:4:
include/linux/fortify-string.h:328:25: warning: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
  328 |                         __write_overflow_field(p_size_field, size);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Expand the memcpy() to the entire structure, though perhaps the correct
solution is to mark all the USB command structures as "__packed".

Reported-by: kernel test robot <lkp@intel.com>
Link: https://lore.kernel.org/lkml/202204142318.vDqjjSFn-lkp@intel.com
Cc: Johan Hovold <johan@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/usb/serial/whiteheat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/serial/whiteheat.c b/drivers/usb/serial/whiteheat.c
index da65d14c9ed5..6e00498843fb 100644
--- a/drivers/usb/serial/whiteheat.c
+++ b/drivers/usb/serial/whiteheat.c
@@ -584,7 +584,7 @@ static int firm_send_command(struct usb_serial_port *port, __u8 command,
 		switch (command) {
 		case WHITEHEAT_GET_DTR_RTS:
 			info = usb_get_serial_port_data(port);
-			memcpy(&info->mcr, command_info->result_buffer,
+			memcpy(info, command_info->result_buffer,
 					sizeof(struct whiteheat_dr_info));
 				break;
 		}
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] USB: serial: Fix heap overflow in WHITEHEAT_GET_DTR_RTS
  2022-04-19  4:17 [PATCH] USB: serial: Fix heap overflow in WHITEHEAT_GET_DTR_RTS Kees Cook
@ 2022-04-20  7:54 ` Johan Hovold
  2022-04-20 12:33   ` Jann Horn
  0 siblings, 1 reply; 5+ messages in thread
From: Johan Hovold @ 2022-04-20  7:54 UTC (permalink / raw)
  To: Kees Cook
  Cc: kernel test robot, Greg Kroah-Hartman, linux-usb, stable,
	linux-kernel, linux-hardening

On Mon, Apr 18, 2022 at 09:17:42PM -0700, Kees Cook wrote:
> This looks like it's harmless, as both the source and the destinations are
> currently the same allocation size (4 bytes) and don't use their padding,
> but if anything were to ever be added after the "mcr" member in "struct
> whiteheat_private", it would be overwritten. The structs both have a
> single u8 "mcr" member, but are 4 bytes in padded size. The memcpy()
> destination was explicitly targeting the u8 member (size 1) with the
> length of the whole structure (size 4), triggering the memcpy buffer
> overflow warning:

Ehh... No. The size of a structure with a single u8 is 1, not 4. There's
nothing wrong with the current code even if the use of memcpy for this
is a bit odd.

> In file included from include/linux/string.h:253,
>                  from include/linux/bitmap.h:11,
>                  from include/linux/cpumask.h:12,
>                  from include/linux/smp.h:13,
>                  from include/linux/lockdep.h:14,
>                  from include/linux/spinlock.h:62,
>                  from include/linux/mmzone.h:8,
>                  from include/linux/gfp.h:6,
>                  from include/linux/slab.h:15,
>                  from drivers/usb/serial/whiteheat.c:17:
> In function 'fortify_memcpy_chk',
>     inlined from 'firm_send_command' at drivers/usb/serial/whiteheat.c:587:4:
> include/linux/fortify-string.h:328:25: warning: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
>   328 |                         __write_overflow_field(p_size_field, size);
>       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

So something is confused here.
 
> Expand the memcpy() to the entire structure, though perhaps the correct
> solution is to mark all the USB command structures as "__packed".

Again no, why would you potentially overwrite the whole structure just to
update a single field? This is just wrong.

And the only structure that needs a __packed which doesn't have it
already is the unused struct whiteheat_dump.

> Reported-by: kernel test robot <lkp@intel.com>
> Link: https://lore.kernel.org/lkml/202204142318.vDqjjSFn-lkp@intel.com
> Cc: Johan Hovold <johan@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Cc: stable@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  drivers/usb/serial/whiteheat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/whiteheat.c b/drivers/usb/serial/whiteheat.c
> index da65d14c9ed5..6e00498843fb 100644
> --- a/drivers/usb/serial/whiteheat.c
> +++ b/drivers/usb/serial/whiteheat.c
> @@ -584,7 +584,7 @@ static int firm_send_command(struct usb_serial_port *port, __u8 command,
>  		switch (command) {
>  		case WHITEHEAT_GET_DTR_RTS:
>  			info = usb_get_serial_port_data(port);
> -			memcpy(&info->mcr, command_info->result_buffer,
> +			memcpy(info, command_info->result_buffer,
>  					sizeof(struct whiteheat_dr_info));
>  				break;
>  		}

Johan

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] USB: serial: Fix heap overflow in WHITEHEAT_GET_DTR_RTS
  2022-04-20  7:54 ` Johan Hovold
@ 2022-04-20 12:33   ` Jann Horn
  2022-04-20 18:11     ` Kees Cook
  0 siblings, 1 reply; 5+ messages in thread
From: Jann Horn @ 2022-04-20 12:33 UTC (permalink / raw)
  To: Kees Cook
  Cc: Johan Hovold, kernel test robot, Greg Kroah-Hartman, linux-usb,
	stable, linux-kernel, linux-hardening

On Wed, Apr 20, 2022 at 10:14 AM Johan Hovold <johan@kernel.org> wrote:
> On Mon, Apr 18, 2022 at 09:17:42PM -0700, Kees Cook wrote:
> > This looks like it's harmless, as both the source and the destinations are
> > currently the same allocation size (4 bytes) and don't use their padding,
> > but if anything were to ever be added after the "mcr" member in "struct
> > whiteheat_private", it would be overwritten. The structs both have a
> > single u8 "mcr" member, but are 4 bytes in padded size. The memcpy()
> > destination was explicitly targeting the u8 member (size 1) with the
> > length of the whole structure (size 4), triggering the memcpy buffer
> > overflow warning:
>
> Ehh... No. The size of a structure with a single u8 is 1, not 4. There's
> nothing wrong with the current code even if the use of memcpy for this
> is a bit odd.
>
> > In file included from include/linux/string.h:253,
> >                  from include/linux/bitmap.h:11,
> >                  from include/linux/cpumask.h:12,
> >                  from include/linux/smp.h:13,
> >                  from include/linux/lockdep.h:14,
> >                  from include/linux/spinlock.h:62,
> >                  from include/linux/mmzone.h:8,
> >                  from include/linux/gfp.h:6,
> >                  from include/linux/slab.h:15,
> >                  from drivers/usb/serial/whiteheat.c:17:
> > In function 'fortify_memcpy_chk',
> >     inlined from 'firm_send_command' at drivers/usb/serial/whiteheat.c:587:4:
> > include/linux/fortify-string.h:328:25: warning: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
> >   328 |                         __write_overflow_field(p_size_field, size);
> >       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> So something is confused here.

So something's going wrong in fortify_memcpy_chk()? It looks like it
is called with constant "size" equal to 1, and the condition
"p_size_field < size" (with an unsigned comparison) is either true
(meaning p_size_field would have to be 0) or not known at compile
time?

The original report says it happened when compiling with
CONFIG_CC_OPTIMIZE_FOR_SIZE=y, maybe that matters?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] USB: serial: Fix heap overflow in WHITEHEAT_GET_DTR_RTS
  2022-04-20 12:33   ` Jann Horn
@ 2022-04-20 18:11     ` Kees Cook
  2022-04-21  7:58       ` Johan Hovold
  0 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2022-04-20 18:11 UTC (permalink / raw)
  To: Jann Horn
  Cc: Johan Hovold, kernel test robot, Greg Kroah-Hartman, linux-usb,
	stable, linux-kernel, linux-hardening

On Wed, Apr 20, 2022 at 02:33:06PM +0200, Jann Horn wrote:
> On Wed, Apr 20, 2022 at 10:14 AM Johan Hovold <johan@kernel.org> wrote:
> > On Mon, Apr 18, 2022 at 09:17:42PM -0700, Kees Cook wrote:
> > > This looks like it's harmless, as both the source and the destinations are
> > > currently the same allocation size (4 bytes) and don't use their padding,
> > > but if anything were to ever be added after the "mcr" member in "struct
> > > whiteheat_private", it would be overwritten. The structs both have a
> > > single u8 "mcr" member, but are 4 bytes in padded size. The memcpy()
> > > destination was explicitly targeting the u8 member (size 1) with the
> > > length of the whole structure (size 4), triggering the memcpy buffer
> > > overflow warning:
> >
> > Ehh... No. The size of a structure with a single u8 is 1, not 4. There's
> > nothing wrong with the current code even if the use of memcpy for this
> > is a bit odd.

I thought that was surprising too, and suspected it was something
specific to the build (as Jann also suggested). I tracked it down[1] to
"-mabi=apcs-gnu", which is from CONFIG_AEABI=n.

whiteheat_private {
        __u8                       mcr;                  /*     0     1 */

        /* size: 4, cachelines: 1, members: 1 */
        /* padding: 3 */
        /* last cacheline: 4 bytes */
};


> >
> > > In file included from include/linux/string.h:253,
> > >                  from include/linux/bitmap.h:11,
> > >                  from include/linux/cpumask.h:12,
> > >                  from include/linux/smp.h:13,
> > >                  from include/linux/lockdep.h:14,
> > >                  from include/linux/spinlock.h:62,
> > >                  from include/linux/mmzone.h:8,
> > >                  from include/linux/gfp.h:6,
> > >                  from include/linux/slab.h:15,
> > >                  from drivers/usb/serial/whiteheat.c:17:
> > > In function 'fortify_memcpy_chk',
> > >     inlined from 'firm_send_command' at drivers/usb/serial/whiteheat.c:587:4:
> > > include/linux/fortify-string.h:328:25: warning: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
> > >   328 |                         __write_overflow_field(p_size_field, size);
> > >       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > So something is confused here.
> 
> So something's going wrong in fortify_memcpy_chk()? It looks like it
> is called with constant "size" equal to 1, and the condition

sizeof(info->mcr) is 1.
sizeof(struct whiteheat_dr_info) (with CONFIG_AEABI=n) is 4.

Given nothing actually uses "struct whiteheat_dr_info", except for the
sizing (har har), I suspect the better solution is just to do:

	info->mcr = command_info->result_buffer[0];

-Kees

[1] https://godbolt.org/z/3YvM1MYWW

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] USB: serial: Fix heap overflow in WHITEHEAT_GET_DTR_RTS
  2022-04-20 18:11     ` Kees Cook
@ 2022-04-21  7:58       ` Johan Hovold
  0 siblings, 0 replies; 5+ messages in thread
From: Johan Hovold @ 2022-04-21  7:58 UTC (permalink / raw)
  To: Kees Cook
  Cc: Jann Horn, kernel test robot, Greg Kroah-Hartman, linux-usb,
	stable, linux-kernel, linux-hardening, Arnd Bergmann

[ +CC: Arnd ]

On Wed, Apr 20, 2022 at 11:11:26AM -0700, Kees Cook wrote:
> On Wed, Apr 20, 2022 at 02:33:06PM +0200, Jann Horn wrote:
> > On Wed, Apr 20, 2022 at 10:14 AM Johan Hovold <johan@kernel.org> wrote:
> > > On Mon, Apr 18, 2022 at 09:17:42PM -0700, Kees Cook wrote:
> > > > This looks like it's harmless, as both the source and the destinations are
> > > > currently the same allocation size (4 bytes) and don't use their padding,
> > > > but if anything were to ever be added after the "mcr" member in "struct
> > > > whiteheat_private", it would be overwritten. The structs both have a
> > > > single u8 "mcr" member, but are 4 bytes in padded size. The memcpy()
> > > > destination was explicitly targeting the u8 member (size 1) with the
> > > > length of the whole structure (size 4), triggering the memcpy buffer
> > > > overflow warning:
> > >
> > > Ehh... No. The size of a structure with a single u8 is 1, not 4. There's
> > > nothing wrong with the current code even if the use of memcpy for this
> > > is a bit odd.
> 
> I thought that was surprising too, and suspected it was something
> specific to the build (as Jann also suggested). I tracked it down[1] to
> "-mabi=apcs-gnu", which is from CONFIG_AEABI=n.
> 
> whiteheat_private {
>         __u8                       mcr;                  /*     0     1 */
> 
>         /* size: 4, cachelines: 1, members: 1 */
>         /* padding: 3 */
>         /* last cacheline: 4 bytes */
> };

I stand corrected, thanks.

Do we have other ABIs that can increase the alignment of structures like
this?

Skimming lore reveals a few subsystems that have started depending on
!OABI to not have to deal with this. Apparently the old ARM ABI is
deprecated in user space since gcc-4.6:

	https://lore.kernel.org/all/20190304193723.657089-1-arnd@arndb.de/

Perhaps time to drop support from the kernel too?

> Given nothing actually uses "struct whiteheat_dr_info", except for the
> sizing (har har), I suspect the better solution is just to do:
> 
> 	info->mcr = command_info->result_buffer[0];

Yeah, that works for now. Ideally, we'd cast the result buffer to struct
whiteheat_dr_info and access its single field. But that's not what
current code does and the above is no less confusing.

Patch applied, thanks.

Johan

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-04-21  7:58 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-19  4:17 [PATCH] USB: serial: Fix heap overflow in WHITEHEAT_GET_DTR_RTS Kees Cook
2022-04-20  7:54 ` Johan Hovold
2022-04-20 12:33   ` Jann Horn
2022-04-20 18:11     ` Kees Cook
2022-04-21  7:58       ` Johan Hovold

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.