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@xxxxxxxxxx> > --- > 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
