[PATCH] mdadm --detail --scan causes SIGABRT

[PATCH] mdadm --detail --scan causes SIGABRT

[Nikhil Kshirsagar]

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


Please find attached a patch to fix BZ 1343809.

Details:
mdadm has a buffer overflow if mdinfo->sys_name needs to store a name
larger than 20 characters.

Core was generated by `mdadm --detail /dev/md0'.
Program terminated with signal 6, Aborted.
#0  0x0000003a93e325e5 in raise (sig=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
64      return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
(gdb) where
#0  0x0000003a93e325e5 in raise (sig=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
#1  0x0000003a93e33dc5 in abort () at abort.c:92
#2  0x0000003a93e704f7 in __libc_message (do_abort=2, fmt=0x3a93f578cf
"*** %s ***: %s terminated\n") at
../sysdeps/unix/sysv/linux/libc_fatal.c:198
#3  0x0000003a93f026d7 in __fortify_fail (msg=0x3a93f57875 "buffer
overflow detected") at fortify_fail.c:32
#4  0x0000003a93f005c0 in __chk_fail () at chk_fail.c:29
#5  0x000000000044fe59 in strcpy (fd=<value optimized out>, devnm=<value
optimized out>, options=<value optimized out>) at
/usr/include/bits/string3.h:105
#6  sysfs_read (fd=<value optimized out>, devnm=<value optimized out>,
options=<value optimized out>) at sysfs.c:272
#7  0x000000000041cdfa in Detail (dev=0x7fffe35f1473 "/dev/md0",
c=0x7fffe35ef590) at Detail.c:106
#8  0x0000000000405ed3 in misc_list (argc=<value optimized out>,
argv=<value optimized out>) at mdadm.c:1747
#9  main (argc=<value optimized out>, argv=<value optimized out>) at
mdadm.c:1425
(gdb)


The line that causes the fault is "sysfs.c" line 272

                strcpy(dev->sys_name, de->d_name);

(gdb) print *de
$9 = {d_ino = 14458, d_off = 14471, d_reclen = 40, d_type = 4 '\004',
  d_name =
"dev-oczpcie_23_0_ssd\000\207\070\000\000\000\000\000\000\264\070\000\000\000\000\000\000(\000\004dev-oczpcie_11_0_ssd\000\264\070\000\000\000\000\000\000\265\070\000\000\000\000\000\000
\000\bsync_action\000\b\265\070\000\000\000\000\000\000\266\070\000\000\000\000\000\000(\000\blast_sync_action\000\000\000\000\b\266\070\000\000\000\000\000\000\267\070\000\000\000\000\000\000
\000\bmismatch_cnt\000\267\070\000\000\000\000\000\000\270\070\000\000\000\000\000\000(\000\bsync_speed_min\000\000\000\000\000\000\b\270\070\000\000\000\000\000\000\271\070\000\000\000\000\000\000(\000\bsync_speed_max\000\000\000\000\000\000\b\271\070\000\000\000\000\000\000\272\070"}
(gdb)

dev-oczpcie_23_0_ssd itself is 20 bytes.

There is no place left for the terminating \0,

(gdb) ptype dev
type = struct mdinfo {
    mdu_array_info_t array;
    mdu_disk_info_t disk;
    __u64 events;
    int uuid[4];
    char name[33];
    long long unsigned int data_offset;
    long long unsigned int new_data_offset;
    long long unsigned int component_size;
    long long unsigned int custom_array_size;
    int reshape_active;
    long long unsigned int reshape_progress;
    int recovery_blocked;
    long long unsigned int space_before;
    long long unsigned int space_after;
    union {
        long long unsigned int resync_start;
        long long unsigned int recovery_start;
    };
    long int bitmap_offset;
    long unsigned int safe_mode_delay;
    int new_level;
    int delta_disks;
    int new_layout;
    int new_chunk;
    int errors;
    long unsigned int cache_size;
    int mismatch_cnt;
    char text_version[50];
    int container_member;
    int container_enough;
    char sys_name[20];             <---  20 .
    struct mdinfo *devs;
    struct mdinfo *next;
    int recovery_fd;
    int state_fd;
    int prev_state;
    int curr_state;
    int next_state;
} *
(gdb)

The patch increases the size of sys_name[] to 32 bytes to match the size
of other device name arrays in the mdadm codebase. A customer reported
this issue in SFDC case 01621749.

Thanks,
nikhil.


[-- Attachment #2: 0001-Fix-for-bz-1343809.patch --]
[-- Type: text/x-patch, Size: 788 bytes --]

From 2c3b5692f8c5933e8746305f589efa4edcc00f3c Mon Sep 17 00:00:00 2001
From: Nikhil Kshirsagar <nkshirsa@redhat.com>
Date: Fri, 10 Jun 2016 08:50:10 +0530
Subject: [PATCH] Fix for bz 1343809.

The sys_name array in the mdinfo structure is 20 bytes of storage.
Increasing the size of this array to 32 bytes.
---
 mdadm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mdadm.h b/mdadm.h
index b597658..eb2333a 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -235,7 +235,7 @@ struct mdinfo {
 	int container_enough; /* flag external handlers can set to
 			       * indicate that subarrays have not enough (-1),
 			       * enough to start (0), or all expected disks (1) */
-	char		sys_name[20];
+	char		sys_name[32];
 	struct mdinfo *devs;
 	struct mdinfo *next;
 
-- 
1.8.3.1

Re: [PATCH] mdadm --detail --scan causes SIGABRT

[Nikhil Kshirsagar]

I forgot to add the 'signed off by' part, request the maintainer to add it during the merge.

Thanks,
Nikhil. 

> On 10-Jun-2016, at 9:50 AM, Nikhil Kshirsagar <nkshirsa@redhat.com> wrote:
> 
> 
> Please find attached a patch to fix BZ 1343809.
> 
> Details:
> mdadm has a buffer overflow if mdinfo->sys_name needs to store a name
> larger than 20 characters.
> 
> Core was generated by `mdadm --detail /dev/md0'.
> Program terminated with signal 6, Aborted.
> #0  0x0000003a93e325e5 in raise (sig=6) at
> ../nptl/sysdeps/unix/sysv/linux/raise.c:64
> 64      return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
> (gdb) where
> #0  0x0000003a93e325e5 in raise (sig=6) at
> ../nptl/sysdeps/unix/sysv/linux/raise.c:64
> #1  0x0000003a93e33dc5 in abort () at abort.c:92
> #2  0x0000003a93e704f7 in __libc_message (do_abort=2, fmt=0x3a93f578cf
> "*** %s ***: %s terminated\n") at
> ../sysdeps/unix/sysv/linux/libc_fatal.c:198
> #3  0x0000003a93f026d7 in __fortify_fail (msg=0x3a93f57875 "buffer
> overflow detected") at fortify_fail.c:32
> #4  0x0000003a93f005c0 in __chk_fail () at chk_fail.c:29
> #5  0x000000000044fe59 in strcpy (fd=<value optimized out>, devnm=<value
> optimized out>, options=<value optimized out>) at
> /usr/include/bits/string3.h:105
> #6  sysfs_read (fd=<value optimized out>, devnm=<value optimized out>,
> options=<value optimized out>) at sysfs.c:272
> #7  0x000000000041cdfa in Detail (dev=0x7fffe35f1473 "/dev/md0",
> c=0x7fffe35ef590) at Detail.c:106
> #8  0x0000000000405ed3 in misc_list (argc=<value optimized out>,
> argv=<value optimized out>) at mdadm.c:1747
> #9  main (argc=<value optimized out>, argv=<value optimized out>) at
> mdadm.c:1425
> (gdb)
> 
> 
> The line that causes the fault is "sysfs.c" line 272
> 
>              strcpy(dev->sys_name, de->d_name);
> 
> (gdb) print *de
> $9 = {d_ino = 14458, d_off = 14471, d_reclen = 40, d_type = 4 '\004',
> d_name =
> "dev-oczpcie_23_0_ssd\000\207\070\000\000\000\000\000\000\264\070\000\000\000\000\000\000(\000\004dev-oczpcie_11_0_ssd\000\264\070\000\000\000\000\000\000\265\070\000\000\000\000\000\000
> \000\bsync_action\000\b\265\070\000\000\000\000\000\000\266\070\000\000\000\000\000\000(\000\blast_sync_action\000\000\000\000\b\266\070\000\000\000\000\000\000\267\070\000\000\000\000\000\000
> \000\bmismatch_cnt\000\267\070\000\000\000\000\000\000\270\070\000\000\000\000\000\000(\000\bsync_speed_min\000\000\000\000\000\000\b\270\070\000\000\000\000\000\000\271\070\000\000\000\000\000\000(\000\bsync_speed_max\000\000\000\000\000\000\b\271\070\000\000\000\000\000\000\272\070"}
> (gdb)
> 
> dev-oczpcie_23_0_ssd itself is 20 bytes.
> 
> There is no place left for the terminating \0,
> 
> (gdb) ptype dev
> type = struct mdinfo {
>  mdu_array_info_t array;
>  mdu_disk_info_t disk;
>  __u64 events;
>  int uuid[4];
>  char name[33];
>  long long unsigned int data_offset;
>  long long unsigned int new_data_offset;
>  long long unsigned int component_size;
>  long long unsigned int custom_array_size;
>  int reshape_active;
>  long long unsigned int reshape_progress;
>  int recovery_blocked;
>  long long unsigned int space_before;
>  long long unsigned int space_after;
>  union {
>      long long unsigned int resync_start;
>      long long unsigned int recovery_start;
>  };
>  long int bitmap_offset;
>  long unsigned int safe_mode_delay;
>  int new_level;
>  int delta_disks;
>  int new_layout;
>  int new_chunk;
>  int errors;
>  long unsigned int cache_size;
>  int mismatch_cnt;
>  char text_version[50];
>  int container_member;
>  int container_enough;
>  char sys_name[20];             <---  20 .
>  struct mdinfo *devs;
>  struct mdinfo *next;
>  int recovery_fd;
>  int state_fd;
>  int prev_state;
>  int curr_state;
>  int next_state;
> } *
> (gdb)
> 
> The patch increases the size of sys_name[] to 32 bytes to match the size
> of other device name arrays in the mdadm codebase. A customer reported
> this issue in SFDC case 01621749.
> 
> Thanks,
> nikhil.
> 
> <0001-Fix-for-bz-1343809.patch>

Re: [PATCH] mdadm --detail --scan causes SIGABRT

[Jes Sorensen]

Nikhil Kshirsagar <nkshirsa@redhat.com> writes:
> Please find attached a patch to fix BZ 1343809.
>
> Details:
> mdadm has a buffer overflow if mdinfo->sys_name needs to store a name
> larger than 20 characters.
>
> Core was generated by `mdadm --detail /dev/md0'.
> Program terminated with signal 6, Aborted.
> #0  0x0000003a93e325e5 in raise (sig=6) at
> ../nptl/sysdeps/unix/sysv/linux/raise.c:64
> 64      return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
> (gdb) where
> #0  0x0000003a93e325e5 in raise (sig=6) at
> ../nptl/sysdeps/unix/sysv/linux/raise.c:64
> #1  0x0000003a93e33dc5 in abort () at abort.c:92
> #2  0x0000003a93e704f7 in __libc_message (do_abort=2, fmt=0x3a93f578cf
> "*** %s ***: %s terminated\n") at
> ../sysdeps/unix/sysv/linux/libc_fatal.c:198
> #3  0x0000003a93f026d7 in __fortify_fail (msg=0x3a93f57875 "buffer
> overflow detected") at fortify_fail.c:32
> #4  0x0000003a93f005c0 in __chk_fail () at chk_fail.c:29
> #5  0x000000000044fe59 in strcpy (fd=<value optimized out>, devnm=<value
> optimized out>, options=<value optimized out>) at
> /usr/include/bits/string3.h:105
> #6  sysfs_read (fd=<value optimized out>, devnm=<value optimized out>,
> options=<value optimized out>) at sysfs.c:272
> #7  0x000000000041cdfa in Detail (dev=0x7fffe35f1473 "/dev/md0",
> c=0x7fffe35ef590) at Detail.c:106
> #8  0x0000000000405ed3 in misc_list (argc=<value optimized out>,
> argv=<value optimized out>) at mdadm.c:1747
> #9  main (argc=<value optimized out>, argv=<value optimized out>) at
> mdadm.c:1425
> (gdb)
>
>
> The line that causes the fault is "sysfs.c" line 272
>
>                 strcpy(dev->sys_name, de->d_name);
>
> (gdb) print *de
> $9 = {d_ino = 14458, d_off = 14471, d_reclen = 40, d_type = 4 '\004',
>   d_name =
> "dev-oczpcie_23_0_ssd\000\207\070\000\000\000\000\000\000\264\070\000\000\000\000\000\000(\000\004dev-oczpcie_11_0_ssd\000\264\070\000\000\000\000\000\000\265\070\000\000\000\000\000\000
> \000\bsync_action\000\b\265\070\000\000\000\000\000\000\266\070\000\000\000\000\000\000(\000\blast_sync_action\000\000\000\000\b\266\070\000\000\000\000\000\000\267\070\000\000\000\000\000\000
> \000\bmismatch_cnt\000\267\070\000\000\000\000\000\000\270\070\000\000\000\000\000\000(\000\bsync_speed_min\000\000\000\000\000\000\b\270\070\000\000\000\000\000\000\271\070\000\000\000\000\000\000(\000\bsync_speed_max\000\000\000\000\000\000\b\271\070\000\000\000\000\000\000\272\070"}
> (gdb)
>
> dev-oczpcie_23_0_ssd itself is 20 bytes.
>
> There is no place left for the terminating \0,

Nikhil,

I am curious, how did you get that long device name? I tried to
reproduce the problem here using /dev/disk/by-id/ devices, but they get
converted to the shorter /dev/sdX names automatically so it doesn't
trigger.

I do not disagree we need to fix this, but I am curious of the real life
scenario how you hit the problem?

Right now I am more wondering whether we should handle this in a more
generic way by allocating an appriopriately sized buffer or bump up the
name buffer the way your patch did it.

Cheers,
Jes

Re: [PATCH] mdadm --detail --scan causes SIGABRT

[Nikhil Kshirsagar]

Hi Jes ,

Would it help to examine the core file ? It's present on the machine and location specified in the bz comments . That's how I saw the data structure that had the issue . Indeed there are other device names where the name does not overflow since they are 32 bytes (which is why I chose this value) or where the name *does* get truncated. However this truncation does not seem to happen for de->d_name which is then copied into dev->sys_name.

As for allocating an appropriate size on the heap instead of a static array it does make sense and I can correct the fix to do that but there are lots of other device names which are static arrays. So which ones do we change ? 

-nikhil. 

> On 10-Jun-2016, at 10:41 PM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
> 
> Nikhil Kshirsagar <nkshirsa@redhat.com> writes:
>> Please find attached a patch to fix BZ 1343809.
>> 
>> Details:
>> mdadm has a buffer overflow if mdinfo->sys_name needs to store a name
>> larger than 20 characters.
>> 
>> Core was generated by `mdadm --detail /dev/md0'.
>> Program terminated with signal 6, Aborted.
>> #0  0x0000003a93e325e5 in raise (sig=6) at
>> ../nptl/sysdeps/unix/sysv/linux/raise.c:64
>> 64      return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig);
>> (gdb) where
>> #0  0x0000003a93e325e5 in raise (sig=6) at
>> ../nptl/sysdeps/unix/sysv/linux/raise.c:64
>> #1  0x0000003a93e33dc5 in abort () at abort.c:92
>> #2  0x0000003a93e704f7 in __libc_message (do_abort=2, fmt=0x3a93f578cf
>> "*** %s ***: %s terminated\n") at
>> ../sysdeps/unix/sysv/linux/libc_fatal.c:198
>> #3  0x0000003a93f026d7 in __fortify_fail (msg=0x3a93f57875 "buffer
>> overflow detected") at fortify_fail.c:32
>> #4  0x0000003a93f005c0 in __chk_fail () at chk_fail.c:29
>> #5  0x000000000044fe59 in strcpy (fd=<value optimized out>, devnm=<value
>> optimized out>, options=<value optimized out>) at
>> /usr/include/bits/string3.h:105
>> #6  sysfs_read (fd=<value optimized out>, devnm=<value optimized out>,
>> options=<value optimized out>) at sysfs.c:272
>> #7  0x000000000041cdfa in Detail (dev=0x7fffe35f1473 "/dev/md0",
>> c=0x7fffe35ef590) at Detail.c:106
>> #8  0x0000000000405ed3 in misc_list (argc=<value optimized out>,
>> argv=<value optimized out>) at mdadm.c:1747
>> #9  main (argc=<value optimized out>, argv=<value optimized out>) at
>> mdadm.c:1425
>> (gdb)
>> 
>> 
>> The line that causes the fault is "sysfs.c" line 272
>> 
>>                strcpy(dev->sys_name, de->d_name);
>> 
>> (gdb) print *de
>> $9 = {d_ino = 14458, d_off = 14471, d_reclen = 40, d_type = 4 '\004',
>>  d_name =
>> "dev-oczpcie_23_0_ssd\000\207\070\000\000\000\000\000\000\264\070\000\000\000\000\000\000(\000\004dev-oczpcie_11_0_ssd\000\264\070\000\000\000\000\000\000\265\070\000\000\000\000\000\000
>> \000\bsync_action\000\b\265\070\000\000\000\000\000\000\266\070\000\000\000\000\000\000(\000\blast_sync_action\000\000\000\000\b\266\070\000\000\000\000\000\000\267\070\000\000\000\000\000\000
>> \000\bmismatch_cnt\000\267\070\000\000\000\000\000\000\270\070\000\000\000\000\000\000(\000\bsync_speed_min\000\000\000\000\000\000\b\270\070\000\000\000\000\000\000\271\070\000\000\000\000\000\000(\000\bsync_speed_max\000\000\000\000\000\000\b\271\070\000\000\000\000\000\000\272\070"}
>> (gdb)
>> 
>> dev-oczpcie_23_0_ssd itself is 20 bytes.
>> 
>> There is no place left for the terminating \0,
> 
> Nikhil,
> 
> I am curious, how did you get that long device name? I tried to
> reproduce the problem here using /dev/disk/by-id/ devices, but they get
> converted to the shorter /dev/sdX names automatically so it doesn't
> trigger.
> 
> I do not disagree we need to fix this, but I am curious of the real life
> scenario how you hit the problem?
> 
> Right now I am more wondering whether we should handle this in a more
> generic way by allocating an appriopriately sized buffer or bump up the
> name buffer the way your patch did it.
> 
> Cheers,
> Jes

Re: [PATCH] mdadm --detail --scan causes SIGABRT

[Jes Sorensen]

Nikhil Kshirsagar <nkshirsa@redhat.com> writes:
> Hi Jes ,
>
> Would it help to examine the core file ? It's present on the machine
> and location specified in the bz comments . That's how I saw the data
> structure that had the issue . Indeed there are other device names
> where the name does not overflow since they are 32 bytes (which is why
> I chose this value) or where the name *does* get truncated. However
> this truncation does not seem to happen for de->d_name which is then
> copied into dev->sys_name.
>
> As for allocating an appropriate size on the heap instead of a static
> array it does make sense and I can correct the fix to do that but
> there are lots of other device names which are static arrays. So which
> ones do we change ?

This is the tricky part, sys_name is used in a lot of places in
different ways.

Do you know if they have a /dev/oczpcie_11_0_ssd on the system, and if
they do, how did that device get created?

Cheers,
Jes

Re: [PATCH] mdadm --detail --scan causes SIGABRT

[Nikhil Kshirsagar]

I think the sosreport should contain the info of the raid devices even though mdadm crashes so we don't see that output in there , there is proc/mdstat and other raid related info  there. Let me check. 

If nothing else I can ask them for proc/mdstat and mdadm -detail again with the working test binary i sent them. 

-Nikhil.


> On 10-Jun-2016, at 11:18 PM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
> 
> Nikhil Kshirsagar <nkshirsa@redhat.com> writes:
>> Hi Jes ,
>> 
>> Would it help to examine the core file ? It's present on the machine
>> and location specified in the bz comments . That's how I saw the data
>> structure that had the issue . Indeed there are other device names
>> where the name does not overflow since they are 32 bytes (which is why
>> I chose this value) or where the name *does* get truncated. However
>> this truncation does not seem to happen for de->d_name which is then
>> copied into dev->sys_name.
>> 
>> As for allocating an appropriate size on the heap instead of a static
>> array it does make sense and I can correct the fix to do that but
>> there are lots of other device names which are static arrays. So which
>> ones do we change ?
> 
> This is the tricky part, sys_name is used in a lot of places in
> different ways.
> 
> Do you know if they have a /dev/oczpcie_11_0_ssd on the system, and if
> they do, how did that device get created?
> 
> Cheers,
> Jes

Re: [PATCH] mdadm --detail --scan causes SIGABRT

[Nikhil Kshirsagar]

Hi Jes,

yes those devices exist,

  /dev/oczpcie_11_0_ssd: using cached size 6251381753 sectors

The devices were created by the OCZ driver.

Thanks,
Nikhil.


On 06/10/2016 11:42 PM, Nikhil Kshirsagar wrote:
> I think the sosreport should contain the info of the raid devices even though mdadm crashes so we don't see that output in there , there is proc/mdstat and other raid related info  there. Let me check. 
>
> If nothing else I can ask them for proc/mdstat and mdadm -detail again with the working test binary i sent them. 
>
> -Nikhil.
>
>
>> On 10-Jun-2016, at 11:18 PM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
>>
>> Nikhil Kshirsagar <nkshirsa@redhat.com> writes:
>>> Hi Jes ,
>>>
>>> Would it help to examine the core file ? It's present on the machine
>>> and location specified in the bz comments . That's how I saw the data
>>> structure that had the issue . Indeed there are other device names
>>> where the name does not overflow since they are 32 bytes (which is why
>>> I chose this value) or where the name *does* get truncated. However
>>> this truncation does not seem to happen for de->d_name which is then
>>> copied into dev->sys_name.
>>>
>>> As for allocating an appropriate size on the heap instead of a static
>>> array it does make sense and I can correct the fix to do that but
>>> there are lots of other device names which are static arrays. So which
>>> ones do we change ?
>> This is the tricky part, sys_name is used in a lot of places in
>> different ways.
>>
>> Do you know if they have a /dev/oczpcie_11_0_ssd on the system, and if
>> they do, how did that device get created?
>>
>> Cheers,
>> Jes

Re: [PATCH] mdadm --detail --scan causes SIGABRT

[Jes Sorensen]

Nikhil Kshirsagar <nkshirsa@redhat.com> writes:
> Please find attached a patch to fix BZ 1343809.
>
> Details:
> mdadm has a buffer overflow if mdinfo->sys_name needs to store a name
> larger than 20 characters.

Nikhil,

I have applied this one, adding the Signed-off-by: line as you
requested and slightly adjusted the patch description.

I would like to include some checking fixes too to avoid us overflowing
the new buffer size with even longer names.

Cheers,
Jes