Skip to contentSkip to conversation

USB: serial: garmin_gps: Use struct_size() and flex_array_size() helpers in pkt_add()

PATCH In review Gustavo A. R. Silva 2 participants 2 messages Jan 20, 2022 – Mar 3, 2022

Subsystems (2)
USB USB serial
lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

PATCH 1/1 USB: serial: garmin_gps: Use struct_size() and flex_array_size() helpers in pkt_add()

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 <[email protected]>
---
 drivers/usb/serial/garmin_gps.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/usb/serial/garmin_gps.c +2 -3
index e5c75944ebb7..1d806c108efb 100644
@@ -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;
Signature · 1 line
2.27.0
maintainer
#1 lore raw
Quoted 4 lines · Gustavo A. R. Silva

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.

Quoted 2 lines

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().

Quoted 12 lines
Link https://github.com/KSPP/linux/issues/160
Link https://github.com/KSPP/linux/issues/174
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/usb/serial/garmin_gps.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
drivers/usb/serial/garmin_gps.c +0 -0
index e5c75944ebb7..1d806c108efb 100644

drivers/usb/serial/garmin_gps.c static int pkt_add(struct garmin_data *garmin_data_p, · line 270

Quoted 6 lines
drivers/usb/serial/garmin_gps.c +1 -2
@@ -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.

drivers/usb/serial/garmin_gps.c static int pkt_add(struct garmin_data *garmin_data_p, · line 275

Quoted 6 lines
drivers/usb/serial/garmin_gps.c +1 -1
 		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.

drivers/usb/serial/garmin_gps.c static int pkt_add(struct garmin_data *garmin_data_p, · line 278

Quoted 2 lines
drivers/usb/serial/garmin_gps.c +0 -0
 		spin_lock_irqsave(&garmin_data_p->lock, flags);
 		garmin_data_p->flags |= FLAGS_QUEUING;

Johan