| Message ID | 20240719114650.1551478-1-arnd@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: virtuser: avoid non-constant format string | expand |
On Fri, Jul 19, 2024 at 1:46 PM Arnd Bergmann <arnd@kernel.org> wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > Using a string variable as an sprintf format is potentially > dangerous, and gcc can warn about this: > > drivers/gpio/gpio-virtuser.c: In function 'gpio_virtuser_dbgfs_init_line_attrs': > drivers/gpio/gpio-virtuser.c:808:9: error: format not a string literal and no format arguments [-Werror=format-security] > 808 | sprintf(data->consumer, id); > | ^~~~~~~ > > Change this instance to use a "%s" format instead to print the string > argument. > > Fixes: 91581c4b3f29 ("gpio: virtuser: new virtual testing driver for the GPIO API") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/gpio/gpio-virtuser.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpio-virtuser.c b/drivers/gpio/gpio-virtuser.c > index 0e0d55da4f01..c55b72e426c7 100644 > --- a/drivers/gpio/gpio-virtuser.c > +++ b/drivers/gpio/gpio-virtuser.c > @@ -805,7 +805,7 @@ static int gpio_virtuser_dbgfs_init_line_attrs(struct device *dev, > return -ENOMEM; > > data->ad.desc = desc; > - sprintf(data->consumer, id); > + sprintf(data->consumer, "%s", id); > atomic_set(&data->irq, 0); > atomic_set(&data->irq_count, 0); > > -- > 2.39.2 > I know this should not happen as the string is checked for length when it is set over configfs but while we're at it: maybe make it 100% correct by using snprintf(data->consumer, sizeof(data->consumer), ... ? Bart
On Fri, Jul 19, 2024, at 16:10, Bartosz Golaszewski wrote: >> diff --git a/drivers/gpio/gpio-virtuser.c b/drivers/gpio/gpio-virtuser.c >> index 0e0d55da4f01..c55b72e426c7 100644 >> --- a/drivers/gpio/gpio-virtuser.c >> +++ b/drivers/gpio/gpio-virtuser.c >> @@ -805,7 +805,7 @@ static int gpio_virtuser_dbgfs_init_line_attrs(struct device *dev, >> return -ENOMEM; >> >> data->ad.desc = desc; >> - sprintf(data->consumer, id); >> + sprintf(data->consumer, "%s", id); >> atomic_set(&data->irq, 0); >> atomic_set(&data->irq_count, 0); >> >> -- >> 2.39.2 >> > > I know this should not happen as the string is checked for length when > it is set over configfs but while we're at it: maybe make it 100% > correct by using snprintf(data->consumer, sizeof(data->consumer), ...? Actually I now think this should just be strscpy(data->consumer, id); There was never a reason to use sprintf() here at all. strscpy() does both the correct size check and avoids treating it as a format string. Arnd
On Fri, Jul 19, 2024 at 4:28 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Fri, Jul 19, 2024, at 16:10, Bartosz Golaszewski wrote: > >> diff --git a/drivers/gpio/gpio-virtuser.c b/drivers/gpio/gpio-virtuser.c > >> index 0e0d55da4f01..c55b72e426c7 100644 > >> --- a/drivers/gpio/gpio-virtuser.c > >> +++ b/drivers/gpio/gpio-virtuser.c > >> @@ -805,7 +805,7 @@ static int gpio_virtuser_dbgfs_init_line_attrs(struct device *dev, > >> return -ENOMEM; > >> > >> data->ad.desc = desc; > >> - sprintf(data->consumer, id); > >> + sprintf(data->consumer, "%s", id); > >> atomic_set(&data->irq, 0); > >> atomic_set(&data->irq_count, 0); > >> > >> -- > >> 2.39.2 > >> > > > > I know this should not happen as the string is checked for length when > > it is set over configfs but while we're at it: maybe make it 100% > > correct by using snprintf(data->consumer, sizeof(data->consumer), ...? > > Actually I now think this should just be > > strscpy(data->consumer, id); > > There was never a reason to use sprintf() here at all. > strscpy() does both the correct size check and avoids > treating it as a format string. > > Arnd Even better! Bart
diff --git a/drivers/gpio/gpio-virtuser.c b/drivers/gpio/gpio-virtuser.c index 0e0d55da4f01..c55b72e426c7 100644 --- a/drivers/gpio/gpio-virtuser.c +++ b/drivers/gpio/gpio-virtuser.c @@ -805,7 +805,7 @@ static int gpio_virtuser_dbgfs_init_line_attrs(struct device *dev, return -ENOMEM; data->ad.desc = desc; - sprintf(data->consumer, id); + sprintf(data->consumer, "%s", id); atomic_set(&data->irq, 0); atomic_set(&data->irq_count, 0);