mdadm: protecting sys_name overflow

PATCH In review Nikhil Kshirsagar 2 participants 3 messages Jun 15, 2016 – Jun 16, 2016 MD RAID

lore

Share & tools

lore

Nikhil Kshirsagar

author

2016-06-15 10:09 UTC #0 lore raw

PATCH 1/1 mdadm: protecting sys_name overflow

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 <[email protected]>

From 705aec84c6abf5b09c4202aec7cade9824ca7f12 Mon Sep 17 00:00:00 2001
From: root <[email protected]>
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(+)

sysfs.c +9 -0

index 8379ca8..68b8b95 100644
@@ -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;

Signature · 1 line

1.8.3.1

Jes Sorensen

2016-06-15 18:40 UTC #1 lore raw

Quoted 26 lines · Nikhil Kshirsagar

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 <[email protected]>
From 705aec84c6abf5b09c4202aec7cade9824ca7f12 Mon Sep 17 00:00:00 2001
From: root <[email protected]>
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(+)
sysfs.c +0 -0
index 8379ca8..68b8b95 100644
@@ -283,6 +283,15 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
 			}
 

sysfs.c struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options) · line 294

Quoted 10 lines

sysfs.c +9 -0

 		}
+                /*  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

Nikhil Kshirsagar

author

2016-06-16 03:56 UTC #2 lore raw

Hi Jes,

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

Thanks,
nikhil.

Quoted 45 lines · Jes Sorensen

Quoted 26 lines · Nikhil Kshirsagar
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 <[email protected]>
From 705aec84c6abf5b09c4202aec7cade9824ca7f12 Mon Sep 17 00:00:00 2001
From: root <[email protected]>
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(+)
sysfs.c +0 -0
index 8379ca8..68b8b95 100644
@@ -283,6 +283,15 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
 			}
 
sysfs.c struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options) · line 294
Quoted 10 lines
sysfs.c +9 -0
 		}
+                /*  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