Bug#958060: mdadm segfaults when run from cron.daily

[Martin Mares <mj@ucw.cz>]

Package: mdadm
Version: 4.1-1
Severity: normal
Tags: patch upstream

Dear maintainers,

when mdadm is run from /etc/cron.daily/mdadm on one of my machines, it
deterministically segfaults.

The cron job runs:

	mdadm --monitor --scan --oneshot

Surprisingly, when I run the same command from a normal root shell, it
finishes correctly. Strace reveals that the difference is that in the first
case, it is run with cwd set to "/", while in the second case cwd is "/root".
And in "/", I have a directory "big", which is also the name of one of my
arrays :) But why is mdadm looking this up in the current directory?

The culprit is the following piece of code in Monitor.c in function Monitor():

        if (mdlist->devname[0] == '/')
                st->devname = xstrdup(mdlist->devname);
        else {
                st->devname = xmalloc(8+strlen(mdlist->devname)+1);
                strcpy(strcpy(st->devname, "/dev/md/"),
                       mdlist->devname);
        }

In my case, the first character of mdlist->devname[0] is not '/', so the
else branch is executed ... which copies "/dev/md" to st->devname and happily
overwrites it with mdlist->devname in a moment. Apparently, the outer strcpy
was meant to be a strcat. (This is fixed by the attached patch.)

However, this solves only one part of the puzzle. Even if mdadm accesses the
wrong name, it should not be a reason for a crash. It crashes on a NULL pointer
dereference in Monitor.c, function check_array():

        fd = open(dev, O_RDONLY);
        if (fd < 0)
                goto disappeared;

        if (st->devnm[0] == 0)
                strcpy(st->devnm, fd2devnm(fd));

Here, open() succeeds on "big", so we call fd2devnm(fd) in lib.c:

	char *fd2devnm(int fd)
	{
		struct stat stb;

		if (fstat(fd, &stb) == 0)
			return stat2devnm(&stb);

		return NULL;
	}

Of course fstat succeeds, so we go here:

	char *stat2devnm(struct stat *st)
	{
		if ((S_IFMT & st->st_mode) != S_IFBLK)
			return NULL;

		return devid2devnm(st->st_rdev);
	}

However "fd" refers to a directory, not to a block device, so we return NULL
to strcpy(). This is a separate bug which would also deserves fixing (at least
by a printing an error message instead of crash), even it is unlikely to be
triggered when the first bug is fixed. (However, it can be triggered for example
by running the command while another instance of mdadm is stopping the array.)

-- Package-specific info:

I believe that the details of arrays are not relevant.

-- System Information:
Debian Release: 10.3
  APT prefers stable-updates
  APT policy: (500, 'stable-updates'), (500, 'stable')
Architecture: amd64 (x86_64)

Kernel: Linux 4.19.0-8-amd64 (SMP w/8 CPU cores)
Locale: LANG=C, LC_CTYPE=cs_CZ.UTF-8 (charmap=UTF-8), LANGUAGE=en_US:en (charmap=UTF-8)
Shell: /bin/sh linked to /bin/bash
Init: systemd (via /run/systemd/system)

Versions of packages mdadm depends on:
ii  debconf [debconf-2.0]  1.5.71
ii  libc6                  2.28-10
ii  lsb-base               10.2019051400
ii  udev                   241-7~deb10u3

Versions of packages mdadm recommends:
ii  kmod                            26-1
ii  postfix [mail-transport-agent]  3.4.8-0+10debu1

Versions of packages mdadm suggests:
pn  dracut-core  <none>

-- debconf information:
  mdadm/start_daemon: true
  mdadm/initrdstart_notinconf: false
  mdadm/initrdstart_msg_errconf:
  mdadm/initrdstart_msg_intro:
  mdadm/initrdstart_msg_errmd:
  mdadm/mail_to: root
* mdadm/initrdstart: all
  mdadm/initrdstart_msg_errexist:
  mdadm/autostart: true
  mdadm/autocheck: true
  mdadm/initrdstart_msg_errblock:

diff -ruN mdadm-4.1.orig/Monitor.c mdadm-4.1/Monitor.c
--- mdadm-4.1.orig/Monitor.c	2018-10-01 20:26:06.000000000 +0200
+++ mdadm-4.1/Monitor.c	2020-04-18 00:08:26.342858997 +0200
@@ -176,7 +176,7 @@
 				st->devname = xstrdup(mdlist->devname);
 			else {
 				st->devname = xmalloc(8+strlen(mdlist->devname)+1);
-				strcpy(strcpy(st->devname, "/dev/md/"),
+				strcat(strcpy(st->devname, "/dev/md/"),
 				       mdlist->devname);
 			}
 			st->next = statelist;