Re: [PATCH 4.9 45/83] staging: vc04_services: prevent integer overflow in create_pagelist()

[Martin Weinelt]

Hi.

On 6/9/19 6:42 PM, Greg Kroah-Hartman wrote:
> From: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> 
> commit ca641bae6da977d638458e78cd1487b6160a2718 upstream.

This commit breaks the kernel build because the vchiq_pagelist_info
struct is not defined in v4.9.182.

It was only added in v4.10, in commit
4807f2c0e684e907c501cb96049809d7a957dbc2.


Best regards,

Martin Weinelt


In file included from ./include/uapi/linux/posix_types.h:4:0,
                 from ./include/uapi/linux/types.h:13,
                 from ./include/linux/compiler.h:224,
                 from ./include/linux/linkage.h:4,
                 from ./include/linux/kernel.h:6,
                 from
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:34:
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c: In
function 'create_pagelist':
./include/linux/stddef.h:7:14: warning: return makes integer from
pointer without a cast [-Wint-conversion]
 #define NULL ((void *)0)
              ^
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:385:10:
note: in expansion of macro 'NULL'
   return NULL;
          ^~~~
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:391:12:
error: invalid application of 'sizeof' to incomplete type 'struct
vchiq_pagelist_info'
     sizeof(struct vchiq_pagelist_info)) /
            ^~~~~~
In file included from ./include/uapi/linux/posix_types.h:4:0,
                 from ./include/uapi/linux/types.h:13,
                 from ./include/linux/compiler.h:224,
                 from ./include/linux/linkage.h:4,
                 from ./include/linux/kernel.h:6,
                 from
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:34:
./include/linux/stddef.h:7:14: warning: return makes integer from
pointer without a cast [-Wint-conversion]
 #define NULL ((void *)0)
              ^
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c:394:10:
note: in expansion of macro 'NULL'
   return NULL;
          ^~~~


> 
> The create_pagelist() "count" parameter comes from the user in
> vchiq_ioctl() and it could overflow.  If you look at how create_page()
> is called in vchiq_prepare_bulk_data(), then the "size" variable is an
> int so it doesn't make sense to allow negatives or larger than INT_MAX.
> 
> I don't know this code terribly well, but I believe that typical values
> of "count" are typically quite low and I don't think this check will
> affect normal valid uses at all.
> 
> The "pagelist_size" calculation can also overflow on 32 bit systems, but
> not on 64 bit systems.  I have added an integer overflow check for that
> as well.
> 
> The Raspberry PI doesn't offer the same level of memory protection that
> x86 does so these sorts of bugs are probably not super critical to fix.
> 
> Fixes: 71bad7f08641 ("staging: add bcm2708 vchiq driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Cc: stable <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
> @@ -381,9 +381,18 @@ create_pagelist(char __user *buf, size_t
>  	int run, addridx, actual_pages;
>          unsigned long *need_release;
>  
> +	if (count >= INT_MAX - PAGE_SIZE)
> +		return NULL;
> +
>  	offset = (unsigned int)buf & (PAGE_SIZE - 1);
>  	num_pages = (count + offset + PAGE_SIZE - 1) / PAGE_SIZE;
>  
> +	if (num_pages > (SIZE_MAX - sizeof(PAGELIST_T) -
> +			 sizeof(struct vchiq_pagelist_info)) /
> +			(sizeof(u32) + sizeof(pages[0]) +
> +			 sizeof(struct scatterlist)))
> +		return NULL;
> +
>  	*ppagelist = NULL;
>  
>  	/* Allocate enough storage to hold the page pointers and the page
>