* [PATCH][next] USB: serial: garmin_gps: Use struct_size() and flex_array_size() helpers in pkt_add()
@ 2022-01-20 0:55 Gustavo A. R. Silva
2022-03-03 12:48 ` Johan Hovold
0 siblings, 1 reply; 2+ messages in thread
From: Gustavo A. R. Silva @ 2022-01-20 0:55 UTC (permalink / raw)
To: Johan Hovold, Greg Kroah-Hartman
Cc: linux-usb, linux-kernel, Gustavo A. R. Silva, linux-hardening
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.
Also, address the following sparse warnings:
drivers/usb/serial/garmin_gps.c:270:31: warning: using sizeof on a flexible structure
Link: https://github.com/KSPP/linux/issues/160
Link: https://github.com/KSPP/linux/issues/174
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.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);
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));
spin_lock_irqsave(&garmin_data_p->lock, flags);
garmin_data_p->flags |= FLAGS_QUEUING;
--
2.27.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH][next] USB: serial: garmin_gps: Use struct_size() and flex_array_size() helpers in pkt_add()
2022-01-20 0:55 [PATCH][next] USB: serial: garmin_gps: Use struct_size() and flex_array_size() helpers in pkt_add() Gustavo A. R. Silva
@ 2022-03-03 12:48 ` Johan Hovold
0 siblings, 0 replies; 2+ messages in thread
From: Johan Hovold @ 2022-03-03 12:48 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, linux-hardening
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@kernel.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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-03-03 12:48 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-20 0:55 [PATCH][next] USB: serial: garmin_gps: Use struct_size() and flex_array_size() helpers in pkt_add() Gustavo A. R. Silva
2022-03-03 12:48 ` 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.