diff mbox series

USB: ene_usb6250: Allocate enough memory for full object

Message ID 20230204183546.never.849-kees@kernel.org (mailing list archive)
State Accepted
Commit ce33e64c1788912976b61314b56935abd4bc97ef
Headers show
Series USB: ene_usb6250: Allocate enough memory for full object | expand

Commit Message

Kees Cook Feb. 4, 2023, 6:35 p.m. UTC
The allocation of PageBuffer is 512 bytes in size, but the dereferencing
of struct ms_bootblock_idi (also size 512) happens at a calculated offset
within the allocation, which means the object could potentially extend
beyond the end of the allocation. Avoid this case by just allocating
enough space to catch any accesses beyond the end. Seen with GCC 13:

../drivers/usb/storage/ene_ub6250.c: In function 'ms_lib_process_bootblock':
../drivers/usb/storage/ene_ub6250.c:1050:44: warning: array subscript 'struct ms_bootblock_idi[0]' is partly outside array bounds of 'unsigned char[512]' [-Warray-bounds=]
 1050 |                         if (le16_to_cpu(idi->wIDIgeneralConfiguration) != MS_IDI_GENERAL_CONF)
      |                                            ^~
../include/uapi/linux/byteorder/little_endian.h:37:51: note: in definition of macro '__le16_to_cpu'
   37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
      |                                                   ^
../drivers/usb/storage/ene_ub6250.c:1050:29: note: in expansion of macro 'le16_to_cpu'
 1050 |                         if (le16_to_cpu(idi->wIDIgeneralConfiguration) != MS_IDI_GENERAL_CONF)
      |                             ^~~~~~~~~~~
In file included from ../drivers/usb/storage/ene_ub6250.c:5:
In function 'kmalloc',
    inlined from 'ms_lib_process_bootblock' at ../drivers/usb/storage/ene_ub6250.c:942:15:
../include/linux/slab.h:580:24: note: at offset [256, 512] into object of size 512 allocated by 'kmalloc_trace'
  580 |                 return kmalloc_trace(
      |                        ^~~~~~~~~~~~~~
  581 |                                 kmalloc_caches[kmalloc_type(flags)][index],
      |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  582 |                                 flags, size);
      |                                 ~~~~~~~~~~~~

Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Cc: usb-storage@lists.one-eyed-alien.net
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/usb/storage/ene_ub6250.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alan Stern Feb. 4, 2023, 7:43 p.m. UTC | #1
On Sat, Feb 04, 2023 at 10:35:46AM -0800, Kees Cook wrote:
> The allocation of PageBuffer is 512 bytes in size, but the dereferencing
> of struct ms_bootblock_idi (also size 512) happens at a calculated offset
> within the allocation, which means the object could potentially extend
> beyond the end of the allocation. Avoid this case by just allocating
> enough space to catch any accesses beyond the end. Seen with GCC 13:

In principle, it would be better to add a runtime check for overflow.  
Doing it this way means that the code could read an invalid value.

In fact, I get the impression that this code tries to load a data 
structure which might straddle a page boundary by reading in just the 
first page.  Either that, or else EntryOffset is always a multiple of 
512 so the error cannot arise.

In any event, it's doubtful that there are very many devices of this 
sort still in use, so it probably doesn't matter.

Alan Stern

> 
> ../drivers/usb/storage/ene_ub6250.c: In function 'ms_lib_process_bootblock':
> ../drivers/usb/storage/ene_ub6250.c:1050:44: warning: array subscript 'struct ms_bootblock_idi[0]' is partly outside array bounds of 'unsigned char[512]' [-Warray-bounds=]
>  1050 |                         if (le16_to_cpu(idi->wIDIgeneralConfiguration) != MS_IDI_GENERAL_CONF)
>       |                                            ^~
> ../include/uapi/linux/byteorder/little_endian.h:37:51: note: in definition of macro '__le16_to_cpu'
>    37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
>       |                                                   ^
> ../drivers/usb/storage/ene_ub6250.c:1050:29: note: in expansion of macro 'le16_to_cpu'
>  1050 |                         if (le16_to_cpu(idi->wIDIgeneralConfiguration) != MS_IDI_GENERAL_CONF)
>       |                             ^~~~~~~~~~~
> In file included from ../drivers/usb/storage/ene_ub6250.c:5:
> In function 'kmalloc',
>     inlined from 'ms_lib_process_bootblock' at ../drivers/usb/storage/ene_ub6250.c:942:15:
> ../include/linux/slab.h:580:24: note: at offset [256, 512] into object of size 512 allocated by 'kmalloc_trace'
>   580 |                 return kmalloc_trace(
>       |                        ^~~~~~~~~~~~~~
>   581 |                                 kmalloc_caches[kmalloc_type(flags)][index],
>       |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   582 |                                 flags, size);
>       |                                 ~~~~~~~~~~~~
> 
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Cc: usb-storage@lists.one-eyed-alien.net
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  drivers/usb/storage/ene_ub6250.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/storage/ene_ub6250.c b/drivers/usb/storage/ene_ub6250.c
> index 6012603f3630..97c66c0d91f4 100644
> --- a/drivers/usb/storage/ene_ub6250.c
> +++ b/drivers/usb/storage/ene_ub6250.c
> @@ -939,7 +939,7 @@ static int ms_lib_process_bootblock(struct us_data *us, u16 PhyBlock, u8 *PageDa
>  	struct ms_lib_type_extdat ExtraData;
>  	struct ene_ub6250_info *info = (struct ene_ub6250_info *) us->extra;
>  
> -	PageBuffer = kmalloc(MS_BYTES_PER_PAGE, GFP_KERNEL);
> +	PageBuffer = kzalloc(MS_BYTES_PER_PAGE * 2, GFP_KERNEL);
>  	if (PageBuffer == NULL)
>  		return (u32)-1;
>  
> -- 
> 2.34.1
>
Kees Cook Feb. 6, 2023, 6:33 p.m. UTC | #2
On Sat, Feb 04, 2023 at 02:43:47PM -0500, Alan Stern wrote:
> On Sat, Feb 04, 2023 at 10:35:46AM -0800, Kees Cook wrote:
> > The allocation of PageBuffer is 512 bytes in size, but the dereferencing
> > of struct ms_bootblock_idi (also size 512) happens at a calculated offset
> > within the allocation, which means the object could potentially extend
> > beyond the end of the allocation. Avoid this case by just allocating
> > enough space to catch any accesses beyond the end. Seen with GCC 13:
> 
> In principle, it would be better to add a runtime check for overflow.  
> Doing it this way means that the code could read an invalid value.
> 
> In fact, I get the impression that this code tries to load a data 
> structure which might straddle a page boundary by reading in just the 
> first page.  Either that, or else EntryOffset is always a multiple of 
> 512 so the error cannot arise.

Yeah, I couldn't figure it out. It seems like it might move in
non-512-byte steps too sometimes? Doubling the allocation (and zero-fill
it) seemed the safest way to cover it.

-Kees
Alan Stern Feb. 6, 2023, 7 p.m. UTC | #3
On Mon, Feb 06, 2023 at 10:33:44AM -0800, Kees Cook wrote:
> On Sat, Feb 04, 2023 at 02:43:47PM -0500, Alan Stern wrote:
> > On Sat, Feb 04, 2023 at 10:35:46AM -0800, Kees Cook wrote:
> > > The allocation of PageBuffer is 512 bytes in size, but the dereferencing
> > > of struct ms_bootblock_idi (also size 512) happens at a calculated offset
> > > within the allocation, which means the object could potentially extend
> > > beyond the end of the allocation. Avoid this case by just allocating
> > > enough space to catch any accesses beyond the end. Seen with GCC 13:
> > 
> > In principle, it would be better to add a runtime check for overflow.  
> > Doing it this way means that the code could read an invalid value.
> > 
> > In fact, I get the impression that this code tries to load a data 
> > structure which might straddle a page boundary by reading in just the 
> > first page.  Either that, or else EntryOffset is always a multiple of 
> > 512 so the error cannot arise.
> 
> Yeah, I couldn't figure it out. It seems like it might move in
> non-512-byte steps too sometimes? Doubling the allocation (and zero-fill
> it) seemed the safest way to cover it.

It hardly seems to matter at this point.  I doubt that any significant 
number of laptops still in operation use the ENE UB6250 reader.  The 
driver was originally written in 2010, and official support for the 
hardware under Windows apparently ended with Windows 7.

Alan Stern
diff mbox series

Patch

diff --git a/drivers/usb/storage/ene_ub6250.c b/drivers/usb/storage/ene_ub6250.c
index 6012603f3630..97c66c0d91f4 100644
--- a/drivers/usb/storage/ene_ub6250.c
+++ b/drivers/usb/storage/ene_ub6250.c
@@ -939,7 +939,7 @@  static int ms_lib_process_bootblock(struct us_data *us, u16 PhyBlock, u8 *PageDa
 	struct ms_lib_type_extdat ExtraData;
 	struct ene_ub6250_info *info = (struct ene_ub6250_info *) us->extra;
 
-	PageBuffer = kmalloc(MS_BYTES_PER_PAGE, GFP_KERNEL);
+	PageBuffer = kzalloc(MS_BYTES_PER_PAGE * 2, GFP_KERNEL);
 	if (PageBuffer == NULL)
 		return (u32)-1;