| Message ID | 20180629025135.12236-1-yuehaibing@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On Fri, Jun 29, 2018 at 5:51 AM, YueHaibing <yuehaibing@huawei.com> wrote: > 'firmware' is a module param which may been longer than firmware_id, > so using strlcpy() to guard against overflows strncat() is against overflow, this does a bit more. > priv->firmware_id[0] = '\0'; ... > if (firmware) /* module parameter */ > - strcpy(priv->firmware_id, firmware); > + strlcpy(priv->firmware_id, firmware, sizeof(priv->firmware_id)); In either case the above '\0' is not needed. But it looks like the intention was to use strncat() / strlcat().
On Sat, Jun 30, 2018 at 12:47 AM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Jun 29, 2018 at 5:51 AM, YueHaibing <yuehaibing@huawei.com> wrote: >> 'firmware' is a module param which may been longer than firmware_id, >> so using strlcpy() to guard against overflows > > strncat() is against overflow, this does a bit more. > >> priv->firmware_id[0] = '\0'; > ... >> if (firmware) /* module parameter */ >> - strcpy(priv->firmware_id, firmware); >> + strlcpy(priv->firmware_id, firmware, sizeof(priv->firmware_id)); > > In either case the above '\0' is not needed. > But it looks like the intention was to use strncat() / strlcat(). Ah, this is under condition, yes. If no parameter supplied, this needs to be clean, but priv is allocated with zeroed memory https://elixir.bootlin.com/linux/latest/source/net/core/dev.c#L8369
On 2018/6/30 5:51, Andy Shevchenko wrote: > On Sat, Jun 30, 2018 at 12:47 AM, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Fri, Jun 29, 2018 at 5:51 AM, YueHaibing <yuehaibing@huawei.com> wrote: >>> 'firmware' is a module param which may been longer than firmware_id, >>> so using strlcpy() to guard against overflows >> >> strncat() is against overflow, this does a bit more. >> >>> priv->firmware_id[0] = '\0'; >> ... >>> if (firmware) /* module parameter */ >>> - strcpy(priv->firmware_id, firmware); >>> + strlcpy(priv->firmware_id, firmware, sizeof(priv->firmware_id)); >> >> In either case the above '\0' is not needed. >> But it looks like the intention was to use strncat() / strlcat(). > > Ah, this is under condition, yes. If no parameter supplied, this needs > to be clean, but > priv is allocated with zeroed memory > https://elixir.bootlin.com/linux/latest/source/net/core/dev.c#L8369 so just remove this line: priv->firmware_id[0] = '\0'; and using strlcpy while 'firmware' NOT null is enough. Will send v2. >
diff --git a/drivers/net/wireless/atmel/atmel.c b/drivers/net/wireless/atmel/atmel.c index b01dc34..604d5ac 100644 --- a/drivers/net/wireless/atmel/atmel.c +++ b/drivers/net/wireless/atmel/atmel.c @@ -1519,7 +1519,7 @@ struct net_device *init_atmel_card(unsigned short irq, unsigned long port, priv->firmware_id[0] = '\0'; priv->firmware_type = fw_type; if (firmware) /* module parameter */ - strcpy(priv->firmware_id, firmware); + strlcpy(priv->firmware_id, firmware, sizeof(priv->firmware_id)); priv->bus_type = card_present ? BUS_TYPE_PCCARD : BUS_TYPE_PCI; priv->station_state = STATION_STATE_DOWN; priv->do_rx_crc = 0;
'firmware' is a module param which may been longer than firmware_id, so using strlcpy() to guard against overflows Signed-off-by: YueHaibing <yuehaibing@huawei.com> --- drivers/net/wireless/atmel/atmel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)