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

[Jes Sorensen <Jes.Sorensen@redhat.com>]

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