diff mbox series

[v8,02/20] gpiolib: cdev: replace strncpy with strscpy

Message ID 20200909102640.1657622-3-warthog618@gmail.com
State Superseded
Headers show
Series gpio: cdev: add uAPI v2 | expand

Commit Message

Kent Gibson Sept. 9, 2020, 10:26 a.m. UTC
Replace usage of strncpy with strscpy to remove -Wstringop-truncation
warnings.

The structs being populated are zeroed, to prevent stack leakage as
they are returned to userspace, so strscpy performs the equivalent
function without the warnings.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Kent Gibson <warthog618@gmail.com>
---

The memset in gpio_desc_to_lineinfo(), in conjunction with the strscpy,
is necessary as strncpy zero pads the remainder of the destination.
It also guarantees that the info cannot leak kernel stack to userspace.
This is useful here, but is even more important for the v2 info, that
this function is changed to generate in a subsequent patch, as that
struct contains padding and attribute arrays that also need to be
initialised.

 drivers/gpio/gpiolib-cdev.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

Comments

Andy Shevchenko Sept. 15, 2020, 10:11 a.m. UTC | #1
On Wed, Sep 9, 2020 at 1:34 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> Replace usage of strncpy with strscpy to remove -Wstringop-truncation

strnspy() with strscpy()

> warnings.
>
> The structs being populated are zeroed, to prevent stack leakage as

structures

> they are returned to userspace, so strscpy performs the equivalent

strscpy()

> function without the warnings.

Isn't this what I commented on like half a year ago and back then
reply was something that there is no warnings or so?

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
>
> The memset in gpio_desc_to_lineinfo(), in conjunction with the strscpy,
> is necessary as strncpy zero pads the remainder of the destination.
> It also guarantees that the info cannot leak kernel stack to userspace.
> This is useful here, but is even more important for the v2 info, that
> this function is changed to generate in a subsequent patch, as that
> struct contains padding and attribute arrays that also need to be
> initialised.
>
>  drivers/gpio/gpiolib-cdev.c | 23 +++++++----------------
>  1 file changed, 7 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index e95e3eab9867..8b012879fe3f 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -752,6 +752,7 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
>         bool ok_for_pinctrl;
>         unsigned long flags;
>
> +       memset(info, 0, sizeof(*info));
>         info->line_offset = gpio_chip_hwgpio(desc);
>         /*
>          * This function takes a mutex so we must check this before taking
> @@ -765,19 +766,11 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
>
>         spin_lock_irqsave(&gpio_lock, flags);
>
> -       if (desc->name) {
> -               strncpy(info->name, desc->name, sizeof(info->name));
> -               info->name[sizeof(info->name) - 1] = '\0';
> -       } else {
> -               info->name[0] = '\0';
> -       }
> +       if (desc->name)
> +               strscpy(info->name, desc->name, sizeof(info->name));
>
> -       if (desc->label) {
> -               strncpy(info->consumer, desc->label, sizeof(info->consumer));
> -               info->consumer[sizeof(info->consumer) - 1] = '\0';
> -       } else {
> -               info->consumer[0] = '\0';
> -       }
> +       if (desc->label)
> +               strscpy(info->consumer, desc->label, sizeof(info->consumer));
>
>         /*
>          * Userspace only need to know that the kernel is using this GPIO so
> @@ -841,12 +834,10 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>
>                 memset(&chipinfo, 0, sizeof(chipinfo));
>
> -               strncpy(chipinfo.name, dev_name(&gdev->dev),
> +               strscpy(chipinfo.name, dev_name(&gdev->dev),
>                         sizeof(chipinfo.name));
> -               chipinfo.name[sizeof(chipinfo.name)-1] = '\0';
> -               strncpy(chipinfo.label, gdev->label,
> +               strscpy(chipinfo.label, gdev->label,
>                         sizeof(chipinfo.label));
> -               chipinfo.label[sizeof(chipinfo.label)-1] = '\0';
>                 chipinfo.lines = gdev->ngpio;
>                 if (copy_to_user(ip, &chipinfo, sizeof(chipinfo)))
>                         return -EFAULT;
> --
> 2.28.0
>
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index e95e3eab9867..8b012879fe3f 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -752,6 +752,7 @@  static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 	bool ok_for_pinctrl;
 	unsigned long flags;
 
+	memset(info, 0, sizeof(*info));
 	info->line_offset = gpio_chip_hwgpio(desc);
 	/*
 	 * This function takes a mutex so we must check this before taking
@@ -765,19 +766,11 @@  static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
 
 	spin_lock_irqsave(&gpio_lock, flags);
 
-	if (desc->name) {
-		strncpy(info->name, desc->name, sizeof(info->name));
-		info->name[sizeof(info->name) - 1] = '\0';
-	} else {
-		info->name[0] = '\0';
-	}
+	if (desc->name)
+		strscpy(info->name, desc->name, sizeof(info->name));
 
-	if (desc->label) {
-		strncpy(info->consumer, desc->label, sizeof(info->consumer));
-		info->consumer[sizeof(info->consumer) - 1] = '\0';
-	} else {
-		info->consumer[0] = '\0';
-	}
+	if (desc->label)
+		strscpy(info->consumer, desc->label, sizeof(info->consumer));
 
 	/*
 	 * Userspace only need to know that the kernel is using this GPIO so
@@ -841,12 +834,10 @@  static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 
 		memset(&chipinfo, 0, sizeof(chipinfo));
 
-		strncpy(chipinfo.name, dev_name(&gdev->dev),
+		strscpy(chipinfo.name, dev_name(&gdev->dev),
 			sizeof(chipinfo.name));
-		chipinfo.name[sizeof(chipinfo.name)-1] = '\0';
-		strncpy(chipinfo.label, gdev->label,
+		strscpy(chipinfo.label, gdev->label,
 			sizeof(chipinfo.label));
-		chipinfo.label[sizeof(chipinfo.label)-1] = '\0';
 		chipinfo.lines = gdev->ngpio;
 		if (copy_to_user(ip, &chipinfo, sizeof(chipinfo)))
 			return -EFAULT;