[PATCH 02/18] block/aoenet: change strncpy+truncation to strlcpy

[PATCH 02/18] block/aoenet: change strncpy+truncation to strlcpy

[Dominique Martinet]

Generated by scripts/coccinelle/misc/strncpy_truncation.cocci

Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---

Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch

 drivers/block/aoe/aoenet.c |  3 +--
 drivers/gpio/gpiolib.c     | 12 ++++--------
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/block/aoe/aoenet.c b/drivers/block/aoe/aoenet.c
index 63773a90581d..1b2871056f8f 100644
--- a/drivers/block/aoe/aoenet.c
+++ b/drivers/block/aoe/aoenet.c
@@ -39,8 +39,7 @@ static struct ktstate kts;
 #ifndef MODULE
 static int __init aoe_iflist_setup(char *str)
 {
-	strncpy(aoe_iflist, str, IFLISTSZ);
-	aoe_iflist[IFLISTSZ - 1] = '\0';
+	strlcpy(aoe_iflist, str, IFLISTSZ);
 	return 1;
 }
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e11a3bb03820..be08277699ff 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1015,12 +1015,10 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 
 		memset(&chipinfo, 0, sizeof(chipinfo));
 
-		strncpy(chipinfo.name, dev_name(&gdev->dev),
+		strlcpy(chipinfo.name, dev_name(&gdev->dev),
 			sizeof(chipinfo.name));
-		chipinfo.name[sizeof(chipinfo.name)-1] = '\0';
-		strncpy(chipinfo.label, gdev->label,
+		strlcpy(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;
@@ -1036,16 +1034,14 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 
 		desc = &gdev->descs[lineinfo.line_offset];
 		if (desc->name) {
-			strncpy(lineinfo.name, desc->name,
+			strlcpy(lineinfo.name, desc->name,
 				sizeof(lineinfo.name));
-			lineinfo.name[sizeof(lineinfo.name)-1] = '\0';
 		} else {
 			lineinfo.name[0] = '\0';
 		}
 		if (desc->label) {
-			strncpy(lineinfo.consumer, desc->label,
+			strlcpy(lineinfo.consumer, desc->label,
 				sizeof(lineinfo.consumer));
-			lineinfo.consumer[sizeof(lineinfo.consumer)-1] = '\0';
 		} else {
 			lineinfo.consumer[0] = '\0';
 		}
-- 
2.17.1

Re: [PATCH 02/18] block/aoenet: change strncpy+truncation to strlcpy

[Ed Cashin]

[-- Attachment #1: Type: text/plain, Size: 2974 bytes --]

OK, thanks.

On Thu, Jul 12, 2018 at 9:33 PM Dominique Martinet <asmadeus@codewreck.org>
wrote:

> Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
>
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> ---
>
> Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
> first patch of the serie) for the motivation behind this patch
>
>  drivers/block/aoe/aoenet.c |  3 +--
>  drivers/gpio/gpiolib.c     | 12 ++++--------
>  2 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/block/aoe/aoenet.c b/drivers/block/aoe/aoenet.c
> index 63773a90581d..1b2871056f8f 100644
> --- a/drivers/block/aoe/aoenet.c
> +++ b/drivers/block/aoe/aoenet.c
> @@ -39,8 +39,7 @@ static struct ktstate kts;
>  #ifndef MODULE
>  static int __init aoe_iflist_setup(char *str)
>  {
> -       strncpy(aoe_iflist, str, IFLISTSZ);
> -       aoe_iflist[IFLISTSZ - 1] = '\0';
> +       strlcpy(aoe_iflist, str, IFLISTSZ);
>         return 1;
>  }
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index e11a3bb03820..be08277699ff 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1015,12 +1015,10 @@ static long gpio_ioctl(struct file *filp, unsigned
> int cmd, unsigned long arg)
>
>                 memset(&chipinfo, 0, sizeof(chipinfo));
>
> -               strncpy(chipinfo.name, dev_name(&gdev->dev),
> +               strlcpy(chipinfo.name, dev_name(&gdev->dev),
>                         sizeof(chipinfo.name));
> -               chipinfo.name[sizeof(chipinfo.name)-1] = '\0';
> -               strncpy(chipinfo.label, gdev->label,
> +               strlcpy(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;
> @@ -1036,16 +1034,14 @@ static long gpio_ioctl(struct file *filp, unsigned
> int cmd, unsigned long arg)
>
>                 desc = &gdev->descs[lineinfo.line_offset];
>                 if (desc->name) {
> -                       strncpy(lineinfo.name, desc->name,
> +                       strlcpy(lineinfo.name, desc->name,
>                                 sizeof(lineinfo.name));
> -                       lineinfo.name[sizeof(lineinfo.name)-1] = '\0';
>                 } else {
>                         lineinfo.name[0] = '\0';
>                 }
>                 if (desc->label) {
> -                       strncpy(lineinfo.consumer, desc->label,
> +                       strlcpy(lineinfo.consumer, desc->label,
>                                 sizeof(lineinfo.consumer));
> -                       lineinfo.consumer[sizeof(lineinfo.consumer)-1] =
> '\0';
>                 } else {
>                         lineinfo.consumer[0] = '\0';
>                 }
> --
> 2.17.1
>
>

-- 
  Ed Cashin <ecashin@noserose.net>

[-- Attachment #2: Type: text/html, Size: 4981 bytes --]

Re: [PATCH 02/18] block/aoenet: change strncpy+truncation to strlcpy

[Jens Axboe]

On 7/12/18 7:25 PM, Dominique Martinet wrote:
> Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
> 
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> ---
> 
> Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
> first patch of the serie) for the motivation behind this patch

This is a weird combination in terms of a single patch, why is
it patching aoe and gpiolib in one patch?

-- 
Jens Axboe

Re: [PATCH 02/18] block/aoenet: change strncpy+truncation to strlcpy

[Dominique Martinet]

Jens Axboe wrote on Fri, Jul 13, 2018:
> This is a weird combination in terms of a single patch, why is
> it patching aoe and gpiolib in one patch?

I must have failed some git add command, I didn't realize they were
together until you pointed it out just now.

This has been discussed more lengthly on the netdev side but the whole
patch serie has been (quite) a bit too hasty; on one side I've been
given some interesting feedback that I would not have gotten just
sending the coccinelle patch first but I really should have waited for
that to land first and most importantly spend more time on each
individual patch, this was rude of me.

I will make sure they are split in a v2, and send them individually
(well, per big component) as self-explaining patches rather than as a
block, once the coccinelle patch has been accepted. 


Thank you for the review,
-- 
Dominique Martinet

[PATCH 15/18] blktrace: change strncpy+truncation to strlcpy

[Dominique Martinet]

Using strlcpy fixes this new gcc warning:
    kernel/trace/blktrace.c: In function ‘do_blk_trace_setup’:
    kernel/trace/blktrace.c:497:2: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation]
      strncpy(buts->name, name, BLKTRACE_BDEV_SIZE);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Generated by scripts/coccinelle/misc/strncpy_truncation.cocci

Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
---

Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
first patch of the serie) for the motivation behind this patch

 kernel/trace/blktrace.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 987d9a9ae283..2478d9838eab 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -494,8 +494,7 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
 	if (!buts->buf_size || !buts->buf_nr)
 		return -EINVAL;
 
-	strncpy(buts->name, name, BLKTRACE_BDEV_SIZE);
-	buts->name[BLKTRACE_BDEV_SIZE - 1] = '\0';
+	strlcpy(buts->name, name, BLKTRACE_BDEV_SIZE);
 
 	/*
 	 * some device names have larger paths - convert the slashes
-- 
2.17.1

Re: [PATCH 15/18] blktrace: change strncpy+truncation to strlcpy

[Steven Rostedt]

Jens,

I noticed this old patch in my inbox. It looks like a legit cleanup.
Want to take it?

-- Steve


On Fri, 13 Jul 2018 03:26:02 +0200
Dominique Martinet <asmadeus@codewreck.org> wrote:

> Using strlcpy fixes this new gcc warning:
>     kernel/trace/blktrace.c: In function ‘do_blk_trace_setup’:
>     kernel/trace/blktrace.c:497:2: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation]
>       strncpy(buts->name, name, BLKTRACE_BDEV_SIZE);
>       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Generated by scripts/coccinelle/misc/strncpy_truncation.cocci
> 
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> ---
> 
> Please see https://marc.info/?l=linux-kernel&m=153144450722324&w=2 (the
> first patch of the serie) for the motivation behind this patch
> 
>  kernel/trace/blktrace.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index 987d9a9ae283..2478d9838eab 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -494,8 +494,7 @@ static int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
>  	if (!buts->buf_size || !buts->buf_nr)
>  		return -EINVAL;
>  
> -	strncpy(buts->name, name, BLKTRACE_BDEV_SIZE);
> -	buts->name[BLKTRACE_BDEV_SIZE - 1] = '\0';
> +	strlcpy(buts->name, name, BLKTRACE_BDEV_SIZE);
>  
>  	/*
>  	 * some device names have larger paths - convert the slashes

Re: [PATCH 15/18] blktrace: change strncpy+truncation to strlcpy

[Jens Axboe]

On 3/14/19 7:37 PM, Steven Rostedt wrote:
> 
> Jens,
> 
> I noticed this old patch in my inbox. It looks like a legit cleanup.
> Want to take it?

Indeed, I've applied it. Thanks!

-- 
Jens Axboe

Re: [PATCH 15/18] blktrace: change strncpy+truncation to strlcpy

[Dominique Martinet]

Jens, Steven,

Jens Axboe wrote on Thu, Mar 14, 2019:
> On 3/14/19 7:37 PM, Steven Rostedt wrote:
> > I noticed this old patch in my inbox. It looks like a legit cleanup.
> > Want to take it?
> 
> Indeed, I've applied it. Thanks!

Thanks. I am terribly sorry about this patch series to be honest, I did
not prepare it properly and sent too many generic patches at once but
more importantly some were unsafe (strlcpy expects the input string to
be validly formatted, because it basically does strlen() on it to check
how much hasn't been copied for its return value)
I was pointed out strscpy instead as a safer alternative.

In this case `name` comes from bdevname() which is disk_name() in
block/partition-generic.c which is a snprintf, so we are guaranted
null truncation from there and it should be OK, but I wanted to check
and point it out.


Anyway, thanks!
-- 
Dominique

Re: [PATCH 15/18] blktrace: change strncpy+truncation to strlcpy

[Jens Axboe]

On 3/15/19 12:30 AM, Dominique Martinet wrote:
> Jens, Steven,
> 
> Jens Axboe wrote on Thu, Mar 14, 2019:
>> On 3/14/19 7:37 PM, Steven Rostedt wrote:
>>> I noticed this old patch in my inbox. It looks like a legit cleanup.
>>> Want to take it?
>>
>> Indeed, I've applied it. Thanks!
> 
> Thanks. I am terribly sorry about this patch series to be honest, I did
> not prepare it properly and sent too many generic patches at once but
> more importantly some were unsafe (strlcpy expects the input string to
> be validly formatted, because it basically does strlen() on it to check
> how much hasn't been copied for its return value)
> I was pointed out strscpy instead as a safer alternative.
> 
> In this case `name` comes from bdevname() which is disk_name() in
> block/partition-generic.c which is a snprintf, so we are guaranted
> null truncation from there and it should be OK, but I wanted to check
> and point it out.

Dropped.

-- 
Jens Axboe