| Message ID | 20181116011036.11483-1-cai@gmx.us (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [v2] mlx4: fix a compilation warning in sysfs.c | expand |
On Thu, Nov 15, 2018 at 08:10:36PM -0500, Qian Cai wrote: > drivers/infiniband/hw/mlx4/sysfs.c:360:2: warning: 'strncpy' output may be > truncated copying 8 bytes from a string of length 31 [-Wstringop-truncation] > > Signed-off-by: Qian Cai <cai@gmx.us> > > Changes since v2: > * Used strscpy() instead of snprintf() > * Removed an unnecessary statement. > > drivers/infiniband/hw/mlx4/sysfs.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/infiniband/hw/mlx4/sysfs.c b/drivers/infiniband/hw/mlx4/sysfs.c > index 752bdd536130..8cf0d2e4c823 100644 > +++ b/drivers/infiniband/hw/mlx4/sysfs.c > @@ -357,8 +357,7 @@ static void get_name(struct mlx4_ib_dev *dev, char *name, int i, int max) > > /* pci_name format is: bus:dev:func -> xxxx:yy:zz.n */ > strlcpy(name, pci_name(dev->dev->persist->pdev), max); > - strncpy(base_name, name, 8); /*till xxxx:yy:*/ > - base_name[8] = '\0'; > + strscpy(base_name, name, sizeof(base_name)); /*till xxxx:yy:*/ > /* with no ARI only 3 last bits are used so when the fn is higher than 8 > * need to add it to the dev num, so count in the last number will be > * modulo 8 */ > sprintf(name, "%s%.2d.%d", base_name, (i/8), (i%8)); This whole routine is goofy. Why not just do something this: sprintf(name, "%.8s%.2d.%d", pci_name(dev->dev->persist->pdev), (i/8), (i%8)); ? And while you are looking at this, the use of sprintf, not snprintf is wrong too, it should be snprintf(name, max, ...) Jason
On Thu, Nov 22, 2018 at 12:26:59PM -0700, Jason Gunthorpe wrote: > On Thu, Nov 15, 2018 at 08:10:36PM -0500, Qian Cai wrote: > > drivers/infiniband/hw/mlx4/sysfs.c:360:2: warning: 'strncpy' output may be > > truncated copying 8 bytes from a string of length 31 [-Wstringop-truncation] > > > > Signed-off-by: Qian Cai <cai@gmx.us> > > > > Changes since v2: > > * Used strscpy() instead of snprintf() > > * Removed an unnecessary statement. > > > > drivers/infiniband/hw/mlx4/sysfs.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/infiniband/hw/mlx4/sysfs.c b/drivers/infiniband/hw/mlx4/sysfs.c > > index 752bdd536130..8cf0d2e4c823 100644 > > +++ b/drivers/infiniband/hw/mlx4/sysfs.c > > @@ -357,8 +357,7 @@ static void get_name(struct mlx4_ib_dev *dev, char *name, int i, int max) > > > > /* pci_name format is: bus:dev:func -> xxxx:yy:zz.n */ > > strlcpy(name, pci_name(dev->dev->persist->pdev), max); > > - strncpy(base_name, name, 8); /*till xxxx:yy:*/ > > - base_name[8] = '\0'; > > + strscpy(base_name, name, sizeof(base_name)); /*till xxxx:yy:*/ > > /* with no ARI only 3 last bits are used so when the fn is higher than 8 > > * need to add it to the dev num, so count in the last number will be > > * modulo 8 */ > > sprintf(name, "%s%.2d.%d", base_name, (i/8), (i%8)); > > This whole routine is goofy. Why not just do something this: > > sprintf(name, "%.8s%.2d.%d", pci_name(dev->dev->persist->pdev), (i/8), (i%8)); > > ? > > And while you are looking at this, the use of sprintf, not snprintf is > wrong too, it should be snprintf(name, max, ...) What is wrong with sprintf here? base_name is limited, not controllable by user and NULL-terminated. Thanks > > Jason
On Thu, Nov 22, 2018 at 09:34:40PM +0200, Leon Romanovsky wrote: > On Thu, Nov 22, 2018 at 12:26:59PM -0700, Jason Gunthorpe wrote: > > On Thu, Nov 15, 2018 at 08:10:36PM -0500, Qian Cai wrote: > > > drivers/infiniband/hw/mlx4/sysfs.c:360:2: warning: 'strncpy' output may be > > > truncated copying 8 bytes from a string of length 31 [-Wstringop-truncation] > > > > > > Signed-off-by: Qian Cai <cai@gmx.us> > > > > > > Changes since v2: > > > * Used strscpy() instead of snprintf() > > > * Removed an unnecessary statement. > > > > > > drivers/infiniband/hw/mlx4/sysfs.c | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > diff --git a/drivers/infiniband/hw/mlx4/sysfs.c b/drivers/infiniband/hw/mlx4/sysfs.c > > > index 752bdd536130..8cf0d2e4c823 100644 > > > +++ b/drivers/infiniband/hw/mlx4/sysfs.c > > > @@ -357,8 +357,7 @@ static void get_name(struct mlx4_ib_dev *dev, char *name, int i, int max) > > > > > > /* pci_name format is: bus:dev:func -> xxxx:yy:zz.n */ > > > strlcpy(name, pci_name(dev->dev->persist->pdev), max); > > > - strncpy(base_name, name, 8); /*till xxxx:yy:*/ > > > - base_name[8] = '\0'; > > > + strscpy(base_name, name, sizeof(base_name)); /*till xxxx:yy:*/ > > > /* with no ARI only 3 last bits are used so when the fn is higher than 8 > > > * need to add it to the dev num, so count in the last number will be > > > * modulo 8 */ > > > sprintf(name, "%s%.2d.%d", base_name, (i/8), (i%8)); > > > > This whole routine is goofy. Why not just do something this: > > > > sprintf(name, "%.8s%.2d.%d", pci_name(dev->dev->persist->pdev), (i/8), (i%8)); > > > > ? > > > > And while you are looking at this, the use of sprintf, not snprintf is > > wrong too, it should be snprintf(name, max, ...) > > What is wrong with sprintf here? > base_name is limited, not controllable by user and NULL-terminated. Because otherwise the developer has to do tricky math in their head to figure out if the string can overflow or not. From a C standard perspective this code is wrong if it happens to run on some crazy platform that has a 64 bit %d, as the sprintf will need 33 chars. Better to be safe, no reason not to be. Jason
diff --git a/drivers/infiniband/hw/mlx4/sysfs.c b/drivers/infiniband/hw/mlx4/sysfs.c index 752bdd536130..8cf0d2e4c823 100644 --- a/drivers/infiniband/hw/mlx4/sysfs.c +++ b/drivers/infiniband/hw/mlx4/sysfs.c @@ -357,8 +357,7 @@ static void get_name(struct mlx4_ib_dev *dev, char *name, int i, int max) /* pci_name format is: bus:dev:func -> xxxx:yy:zz.n */ strlcpy(name, pci_name(dev->dev->persist->pdev), max); - strncpy(base_name, name, 8); /*till xxxx:yy:*/ - base_name[8] = '\0'; + strscpy(base_name, name, sizeof(base_name)); /*till xxxx:yy:*/ /* with no ARI only 3 last bits are used so when the fn is higher than 8 * need to add it to the dev num, so count in the last number will be * modulo 8 */
drivers/infiniband/hw/mlx4/sysfs.c:360:2: warning: 'strncpy' output may be truncated copying 8 bytes from a string of length 31 [-Wstringop-truncation] Signed-off-by: Qian Cai <cai@gmx.us> --- Changes since v2: * Used strscpy() instead of snprintf() * Removed an unnecessary statement. drivers/infiniband/hw/mlx4/sysfs.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)