[PATCH] mdadm: protecting sys_name overflow

[PATCH] mdadm: protecting sys_name overflow

[Nikhil Kshirsagar]

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

Hello,

Devices with names larger than 31 bytes will overflow the sys_name array.

This patch enables mdadm to fail and log a message if a long device name
is going to cause a buffer overflow.

Signed-off-by: Nikhil Kshirsagar <nkshirsa@redhat.com>

[-- Attachment #2: 0001-Protecting-overflow-of-sys_name.-If-a-long-device-na.patch --]
[-- Type: text/x-patch, Size: 1046 bytes --]

From 705aec84c6abf5b09c4202aec7cade9824ca7f12 Mon Sep 17 00:00:00 2001
From: root <root@nkshirsa.pnq.csb>
Date: Wed, 15 Jun 2016 15:23:12 +0530
Subject: [PATCH] Protecting overflow of sys_name. If a long device name is
 going to cause a buffer overflow, we fail with a log message.

---
 sysfs.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/sysfs.c b/sysfs.c
index 8379ca8..68b8b95 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -283,6 +283,15 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
 			}
 
 		}
+                /*  strlen computes length of string *not* including the terminating null character. */
+
+                if(strlen(de->d_name) >= sizeof(dev->sys_name))
+                {
+                  pr_err("Device name %s larger than currently supported by mdadm\n",de->d_name);
+                  free(dev);
+                  goto abort;
+
+                }
 		strcpy(dev->sys_name, de->d_name);
 		dev->disk.raid_disk = strtoul(buf, &ep, 10);
 		if (*ep) dev->disk.raid_disk = -1;
-- 
1.8.3.1

Re: [PATCH] mdadm: protecting sys_name overflow

[Jes Sorensen]

Nikhil Kshirsagar <nkshirsa@redhat.com> writes:
> Hello,
>
> Devices with names larger than 31 bytes will overflow the sys_name array.
>
> This patch enables mdadm to fail and log a message if a long device name
> is going to cause a buffer overflow.
>
> Signed-off-by: Nikhil Kshirsagar <nkshirsa@redhat.com>
>
> From 705aec84c6abf5b09c4202aec7cade9824ca7f12 Mon Sep 17 00:00:00 2001
> From: root <root@nkshirsa.pnq.csb>
> Date: Wed, 15 Jun 2016 15:23:12 +0530
> Subject: [PATCH] Protecting overflow of sys_name. If a long device name is
>  going to cause a buffer overflow, we fail with a log message.
>
> ---
>  sysfs.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/sysfs.c b/sysfs.c
> index 8379ca8..68b8b95 100644
> --- a/sysfs.c
> +++ b/sysfs.c
> @@ -283,6 +283,15 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
>  			}
>  
>  		}
> +                /*  strlen computes length of string *not* including the terminating null character. */
> +
> +                if(strlen(de->d_name) >= sizeof(dev->sys_name))
> +                {
> +                  pr_err("Device name %s larger than currently supported by mdadm\n",de->d_name);
> +                  free(dev);
> +                  goto abort;
> +
> +                }

Nikhil,

Please respect the coding style of mdadm. Code indents at 8 characters
using a tab, not whitespace, and brackets should be placed correctly.

Just like with kernel code.

Jes

Re: [PATCH] mdadm: protecting sys_name overflow

[Nikhil Kshirsagar]

Hi Jes,

Apologies for the wrong indentation. I will re-post the patch.

Thanks,
nikhil.

On 06/16/2016 12:10 AM, Jes Sorensen wrote:
> Nikhil Kshirsagar <nkshirsa@redhat.com> writes:
>> Hello,
>>
>> Devices with names larger than 31 bytes will overflow the sys_name array.
>>
>> This patch enables mdadm to fail and log a message if a long device name
>> is going to cause a buffer overflow.
>>
>> Signed-off-by: Nikhil Kshirsagar <nkshirsa@redhat.com>
>>
>> From 705aec84c6abf5b09c4202aec7cade9824ca7f12 Mon Sep 17 00:00:00 2001
>> From: root <root@nkshirsa.pnq.csb>
>> Date: Wed, 15 Jun 2016 15:23:12 +0530
>> Subject: [PATCH] Protecting overflow of sys_name. If a long device name is
>>  going to cause a buffer overflow, we fail with a log message.
>>
>> ---
>>  sysfs.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/sysfs.c b/sysfs.c
>> index 8379ca8..68b8b95 100644
>> --- a/sysfs.c
>> +++ b/sysfs.c
>> @@ -283,6 +283,15 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
>>  			}
>>  
>>  		}
>> +                /*  strlen computes length of string *not* including the terminating null character. */
>> +
>> +                if(strlen(de->d_name) >= sizeof(dev->sys_name))
>> +                {
>> +                  pr_err("Device name %s larger than currently supported by mdadm\n",de->d_name);
>> +                  free(dev);
>> +                  goto abort;
>> +
>> +                }
> Nikhil,
>
> Please respect the coding style of mdadm. Code indents at 8 characters
> using a tab, not whitespace, and brackets should be placed correctly.
>
> Just like with kernel code.
>
> Jes

[PATCH] mdadm: protecting sys_name overflow

[Nikhil Kshirsagar]

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

Hello,

(Corrected indentation and code formatting, and re-posting this patch.)

Devices with names larger than 31 bytes will overflow the sys_name array.

This patch enables mdadm to fail and log a message if a long device name
is going to cause a buffer overflow.

Signed-off-by: Nikhil Kshirsagar <nkshirsa@redhat.com>

[-- Attachment #2: 0001-Protecting-overflow-of-sys_name.-If-a-long-device-na.patch --]
[-- Type: text/x-patch, Size: 950 bytes --]

From 8198c463c3199c8207dd16cefac23197b16d8a09 Mon Sep 17 00:00:00 2001
From: Nikhil Kshirsagar <nkshirsa@redhat.com>
Date: Thu, 16 Jun 2016 09:25:07 +0530
Subject: [PATCH] Protecting overflow of sys_name. If a long device name is
 going to cause a buffer overflow, we fail with a log message.

---
 sysfs.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/sysfs.c b/sysfs.c
index 8379ca8..d346fe9 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -283,6 +283,13 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
 			}
 
 		}
+
+		/*  strlen computes length of string *not* including the terminating null character. */
+		if (strlen(de->d_name) >= sizeof(dev->sys_name)) {
+			pr_err("Device name %s larger than currently supported by mdadm\n",de->d_name);
+			free(dev);
+			goto abort;
+		}
 		strcpy(dev->sys_name, de->d_name);
 		dev->disk.raid_disk = strtoul(buf, &ep, 10);
 		if (*ep) dev->disk.raid_disk = -1;
-- 
1.8.3.1