linux-kernel - Re: [PATCH][next] USB: serial: garmin_gps: Use struct_size() and flex_array_size() helpers in pkt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <YiC5KeDM9xJIXi0t@hovoldconsulting.com>
Date:   Thu, 3 Mar 2022 13:48:41 +0100
From:   Johan Hovold <johan@...nel.org>
To:     "Gustavo A. R. Silva" <gustavoars@...nel.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] USB: serial: garmin_gps: Use struct_size() and
 flex_array_size() helpers in pkt_add()

On Wed, Jan 19, 2022 at 06:55:12PM -0600, Gustavo A. R. Silva wrote:
> Make use of the struct_size() and flex_array_size() helpers instead of
> an open-coded version, in order to avoid any potential type mistakes
> or integer overflows that, in the worst scenario, could lead to heap
> overflows.

This motivation doesn't seem to apply to flex_array_size() here.
 
> Also, address the following sparse warnings:
> drivers/usb/serial/garmin_gps.c:270:31: warning: using sizeof on a flexible structure

And this is bogus since the warning is not enabled by default (for a
reason) and would still there with this patch applied since
struct_size() relies on sizeof().

> Link: https://github.com/KSPP/linux/issues/160
> Link: https://github.com/KSPP/linux/issues/174
> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> ---
>  drivers/usb/serial/garmin_gps.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/serial/garmin_gps.c b/drivers/usb/serial/garmin_gps.c
> index e5c75944ebb7..1d806c108efb 100644
> --- a/drivers/usb/serial/garmin_gps.c
> +++ b/drivers/usb/serial/garmin_gps.c
> @@ -267,13 +267,12 @@ static int pkt_add(struct garmin_data *garmin_data_p,
>  
>  	/* process only packets containing data ... */
>  	if (data_length) {
> -		pkt = kmalloc(sizeof(struct garmin_packet)+data_length,
> -								GFP_ATOMIC);
> +		pkt = kmalloc(struct_size(pkt, data, data_length), GFP_ATOMIC);

This bit is ok and would cause kmalloc() to fail also if data_length is
ever close to UINT_MAX.

>  		if (!pkt)
>  			return 0;
>  
>  		pkt->size = data_length;
> -		memcpy(pkt->data, data, data_length);
> +		memcpy(pkt->data, data, flex_array_size(pkt, data, pkt->size));

But I fail to see the point in using flex_array_size() when dealing with
byte arrays. It just makes the code harder to read without any benefit.

First of all, we're dealing with a byte array so flex_array_size() will
never saturate. And even if it did, we'd still overflow whatever buffer
we're copying to.

And if the type of pkt->data were to change to a larger one for some
reason, then using flex_array_size() could even be harmful and result in
information leaks.

>  		spin_lock_irqsave(&garmin_data_p->lock, flags);
>  		garmin_data_p->flags |= FLAGS_QUEUING;

Johan