| Message ID | 20220120005512.GA72984@embeddedor (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Gustavo A. R. Silva |
| Headers | show |
| Series | [next] USB: serial: garmin_gps: Use struct_size() and flex_array_size() helpers in pkt_add() | expand |
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
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;
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(-)