Bug#986332: lsattr on certiain files in /dev results in "stack smashing detected"

[Marc Haber]

Package: e2fsprogs
Version: 1.46.2-1
Severity: normal
File: /usr/bin/lsattr

Hi,

while debugging aide, I have stumbled upon a possible bug in lsattr or
one of the underlying libraries. Doing lsattr on /dev/dri/card0 results
in an abort with "stack smashing detected":

| root@testsid85:~# sudo lsattr /dev/dri/card0
| *** stack smashing detected ***: terminated
| Aborted

Similiar behavior is observed when aide is trying to read the ext2 attrs
of this file, so this is most probably a library issue.

I can provide a backtrace if it helps.

Greetings
Marc


-- System Information:
Debian Release: bullseye/sid
APT prefers unstable
APT policy: (500, 'unstable'), (500, 'testing'), (500, 'stable')
Architecture: amd64 (x86_64)

Kernel: Linux 5.10.0-5-amd64 (SMP w/2 CPU threads)
Locale: LANG=de_DE.utf8, LC_CTYPE=de_DE.utf8 (charmap=UTF-8), LANGUAGE=en
Shell: /bin/sh linked to /usr/bin/dash
Init: systemd (via /run/systemd/system)
LSM: AppArmor: enabled

Versions of packages e2fsprogs depends on:
ii libblkid1 2.36.1-7
ii libc6 2.31-11
ii libcom-err2 1.46.2-1
ii libext2fs2 1.46.2-1
ii libss2 1.46.2-1
ii libuuid1 2.36.1-7
ii logsave 1.46.2-1

Versions of packages e2fsprogs recommends:
pn e2fsprogs-l10n <none>

Versions of packages e2fsprogs suggests:
pn e2fsck-static <none>
pn fuse2fs <none>
pn gpart <none>
ii parted 3.4-1

-- no debconf information

[Bernhard Übelacker]

Dear Maintainer,
tried to locate the exact smashing.
It looks like the ioctl(EXT2_IOC_GETFLAGS) takes an int* parameter,
but writes 8 bytes instead of just sizeof(int) to the given address.

Kind regards,
Bernhard


Old value = (void *) 0xf759b62c03711000
New value = (void *) 0xf759b62c00000000
0x00007ffff7ec0cc7 in ioctl () at ../sysdeps/unix/syscall-template.S:120
120 ../sysdeps/unix/syscall-template.S: Datei oder Verzeichnis nicht gefunden.
1: x/i $pc
=> 0x7ffff7ec0cc7 <ioctl+7>: cmp $0xfffffffffffff001,%rax
(gdb) bt
#0 0x00007ffff7ec0cc7 in ioctl () at ../sysdeps/unix/syscall-template.S:120
#1 0x00007ffff7fbcb17 in fgetflags (name=name@entry=0x7fffffffe83f "/dev/dri/card0", flags=flags@entry=0x7fffffffe3e0) at ../../../../lib/e2p/fgetflags.c:90
#2 0x00005555555554d5 in list_attributes (name=name@entry=0x7fffffffe83f "/dev/dri/card0") at ../../../misc/lsattr.c:85
#3 0x00005555555556c9 in lsattr_args (name=0x7fffffffe83f "/dev/dri/card0") at ../../../misc/lsattr.c:134
#4 0x0000555555555369 in main (argc=<optimized out>, argv=<optimized out>) at ../../../misc/lsattr.c:221

https://sources.debian.org/src/e2fsprogs/1.46.2-1/lib/e2p/fgetflags.c/#L90
https://git.kernel.org/pub/scm/fs/ext2/e2fsprogs.git/tree/lib/e2p/fgetflags.c#n90

[Chris Hofstaedtler]

Hello Bernhard, Marc,

* Bernhard Übelacker <bern...@mailbox.org> [210404 20:32]:

> Dear Maintainer,
> tried to locate the exact smashing.
> It looks like the ioctl(EXT2_IOC_GETFLAGS) takes an int* parameter,
> but writes 8 bytes instead of just sizeof(int) to the given address.

Some more questions:
1) which kernel version is this?
2) /dev/dri is on tmpfs?

Chris

[Marc Haber]

Hi Chris,

1 [2/4021]mh@testsid85:~ $ sudo lsattr /dev/dri/card0
[sudo] password for mh:

*** stack smashing detected ***: terminated
Aborted

134 [3/4022]mh@testsid85:~ $ uname -a
Linux testsid85 5.10.0-5-amd64 #1 SMP Debian 5.10.26-1 (2021-03-27) x86_64 GNU/Linux
[4/4023]mh@testsid85:~ $ stat -f /dev/dri
File: "/dev/dri"
ID: 0 Namelen: 255 Type: tmpfs
Block size: 4096 Fundamental block size: 4096
Blocks: Total: 40336 Free: 40336 Available: 40336
Inodes: Total: 40336 Free: 39600
[5/4024]mh@testsid85:~ $

Other /dev device nods can be lsattr'd without error.

Greetings
Marc

--
-----------------------------------------------------------------------------
Marc Haber | "I don't trust Computers. They | Mailadresse im Header
Leimen, Germany | lose things." Winona Ryder | Fon: *49 6224 1600402
Nordisch by Nature | How to make an American Quilt | Fax: *49 6224 1600421

[Bernhard Übelacker]

Hello Chris,


Am 04.04.21 um 22:33 schrieb Chris Hofstaedtler:
> Hello Bernhard, Marc,

> Some more questions:
> 1) which kernel version is this?

My test was just inside a temporary VM with current testing.
But I can still reproduce this with current testing kernel at the host too:
Linux rechner 5.10.0-5-amd64 #1 SMP Debian 5.10.24-1 (2021-03-19) x86_64 GNU/Linux

> 2) /dev/dri is on tmpfs?

bernhard@rechner:~$ mount
udev on /dev type devtmpfs (rw,nosuid,noexec,relatime,size=8075552k,nr_inodes=2018888,mode=755)
...


Kind regards,
Bernhard

[Chris Hofstaedtler]

Hi Marc,

thanks for the followup.

* Marc Haber <mh+debi...@zugschlus.de> [210404 22:03]:

> On Sun, Apr 04, 2021 at 10:33:46PM +0200, Chris Hofstaedtler wrote:
> > * Bernhard Übelacker <bern...@mailbox.org> [210404 20:32]:
> > > Dear Maintainer,
> > > tried to locate the exact smashing.
> > > It looks like the ioctl(EXT2_IOC_GETFLAGS) takes an int* parameter,
> > > but writes 8 bytes instead of just sizeof(int) to the given address.
> >
> > Some more questions:
> > 1) which kernel version is this?
> > 2) /dev/dri is on tmpfs?
>
> 1 [2/4021]mh@testsid85:~ $ sudo lsattr /dev/dri/card0
> [sudo] password for mh:
> *** stack smashing detected ***: terminated
> Aborted
> 134 [3/4022]mh@testsid85:~ $ uname -a
> Linux testsid85 5.10.0-5-amd64 #1 SMP Debian 5.10.26-1 (2021-03-27) x86_64 GNU/Linux
> [4/4023]mh@testsid85:~ $ stat -f /dev/dri
> File: "/dev/dri"
> ID: 0 Namelen: 255 Type: tmpfs
> Block size: 4096 Fundamental block size: 4096
> Blocks: Total: 40336 Free: 40336 Available: 40336
> Inodes: Total: 40336 Free: 39600
> [5/4024]mh@testsid85:~ $
>
> Other /dev device nods can be lsattr'd without error.

I was wondering about changes since buster, and indeed:

Upstream commit 40ea4628 [1] removes the lstat call which shielded
the ioctl call later on. On buster, lsattr /dev/dri/card0 just
gives:
lsattr: Operation not supported While reading flags on /dev/dri/card0

(Even with Linux 5.10.0-0.bpo.3-amd64.)


Now, for the actual issue:

AFAICT, for /dev/dri/card0 the ioctl ends up in the kernel's
drm_ioctl [2], which will blindly call copy_to_user assuming the
output size is the same as the input size (8 bytes). This is wrong
for FS_IOC_GETFLAGS, at least for normal files.

Maybe the best thing is to put the lstat check back in?
Or maybe lsattr should expect that the kernel might actually use the
8 bytes? I have checked various fs ioctl functions, and they all
seem to return 4 bytes, except for orangefs [3] ...

Chris

[1] https://git.kernel.org/pub/scm/fs/ext2/e2fsprogs.git/commit/lib/e2p/fgetflags.c?id=40ea4628ba1b55f8eba311f12399d039698dbeeb
[2] https://elixir.bootlin.com/linux/v5.10.27/source/drivers/gpu/drm/drm_ioctl.c#L888
[3] https://elixir.bootlin.com/linux/v5.10.27/source/fs/orangefs/file.c#L378

[Theodore Ts'o]

On Mon, Apr 05, 2021 at 12:46:48AM +0200, Chris Hofstaedtler wrote:
>
> AFAICT, for /dev/dri/card0 the ioctl ends up in the kernel's
> drm_ioctl [2], which will blindly call copy_to_user assuming the
> output size is the same as the input size (8 bytes). This is wrong
> for FS_IOC_GETFLAGS, at least for normal files.
>
> Maybe the best thing is to put the lstat check back in?
> Or maybe lsattr should expect that the kernel might actually use the
> 8 bytes? I have checked various fs ioctl functions, and they all
> seem to return 4 bytes, except for orangefs [3] ...

What's going on is that apparently there is an overlap between the
ioctl code FS_IOC_GETFLAGS (aka EXT2_IOC_GETFLAGS) and some ioctl code
used by device driver responding to /dev/dri/card0, in drm_ioctl. I
had the vague thought that at some point, we might be able to set and
get file system flags on device files, which is why I removed the
lstat check. I wasn't counting on the fact that there would be ioctl
code collisions --- which in retrospect, was hopelessly optimistic on
my part.

So yeah, we need to put the lstat check back in.

I checked fs/orange/file.c and it is also using 4 bytes (int is always
32 bits even on 64-bit platforms):

if (cmd == FS_IOC_GETFLAGS) {
ret = orangefs_getflags(inode, &uval);
if (ret)
return ret;
gossip_debug(GOSSIP_FILE_DEBUG,
"orangefs_ioctl: FS_IOC_GETFLAGS: %llu\n",
(unsigned long long)uval);
return put_user(uval, (int __user *)arg);
^^^^^^^^^^^^^

% cat /tmp/foo.c
#include <unistd.h>
#include <stdio.h>

int main(int argc, char **argv)
{
printf("size of int: %d\n", sizeof(int));
return 0;
}
% cc -o /tmp/foo /tmp/foo.c
% /tmp/foo
size of int: 4

Fortunately, the fortify compile option detectsd the stack smash, so
it's not critical that we get this fixed ASAP, but we ultimately do
need to put the lstat check back in.

- Ted

[Guillem Jover]

[ Just upgraded my server to bullseye, and aide started to fail, now
noticed this report, after also hitting the error via lsattr. ]

On Mon, 2021-04-05 at 17:35:54 -0400, Theodore Ts'o wrote:
> On Mon, Apr 05, 2021 at 12:46:48AM +0200, Chris Hofstaedtler wrote:
> > AFAICT, for /dev/dri/card0 the ioctl ends up in the kernel's
> > drm_ioctl [2], which will blindly call copy_to_user assuming the
> > output size is the same as the input size (8 bytes). This is wrong
> > for FS_IOC_GETFLAGS, at least for normal files.
> >
> > Maybe the best thing is to put the lstat check back in?
> > Or maybe lsattr should expect that the kernel might actually use the
> > 8 bytes? I have checked various fs ioctl functions, and they all
> > seem to return 4 bytes, except for orangefs [3] ...
>
> What's going on is that apparently there is an overlap between the
> ioctl code FS_IOC_GETFLAGS (aka EXT2_IOC_GETFLAGS) and some ioctl code
> used by device driver responding to /dev/dri/card0, in drm_ioctl. I
> had the vague thought that at some point, we might be able to set and
> get file system flags on device files, which is why I removed the
> lstat check. I wasn't counting on the fact that there would be ioctl
> code collisions --- which in retrospect, was hopelessly optimistic on
> my part.

So, while I was trying to debug this in aide on my server, after
having added few aide ignore entries for specific devices, at some
point while within gdb, I suddenly lost the ssh connection as the
server suddenly rebooted, for which I was very puzzled, and paranoid!

What you mention above, of colliding IOCTL codes would actually
explain the cause of the reboot, if it ends up hitting an IOCTL
setting some wrong state somewhere.

I also noticed this being an issue with at least /dev/kfd, but I'm not
very eager to try this again there to determine what other devices
this might affect, if this can result in sending random commands to
random devices on the live server. :/

> So yeah, we need to put the lstat check back in.

> Fortunately, the fortify compile option detectsd the stack smash, so
> it's not critical that we get this fixed ASAP, but we ultimately do
> need to put the lstat check back in.

I'd appreciate if this could be done sooner rather than later, and it
seems to me it probably deserves a higher severity.

(I've for now disabled aide to avoid any crashes or strange behavior.)

Thanks,
Guillem

[Chris Hofstaedtler]

* Guillem Jover <gui...@hadrons.org> [210424 16:53]:

> So, while I was trying to debug this in aide on my server, after
> having added few aide ignore entries for specific devices, at some
> point while within gdb, I suddenly lost the ssh connection as the
> server suddenly rebooted, for which I was very puzzled, and paranoid!

I tried to reproduce this, and stumbled upon the watchdog and
watchdog0 files. If they are the problem, well...

echo > /dev/watchdog
sleep 30
# notice your machine is rebooting

That would probably by design. I guess its well understood that
writing to random "files" in /dev is a bad plan; it appears
-opening- then is, too.

Chris

[Theodore Ts'o]

tags 986332 +pending
thanks

The following commit should fix things; it will be in the next release
of e2fsprogs.

- Ted

commit a3f844da91f0c01209a5d778a5af57fabe245332
Author: Theodore Ts'o <ty...@mit.edu>
Date: Fri Jul 16 22:31:26 2021 -0400

libe2p: use stat to prevent calling EXT2_IOC_[GS]ETFLAGS on devices

Some devices can react badly to the EXT2_IOC_[GS]ETFLAGS ioctls, since
ioctl codes are not guaranteed to be unique across different device
drivers and file systems.

Addresses-Debian-Bug: #986332
Signed-off-by: Theodore Ts'o <ty...@mit.edu>

diff --git a/lib/e2p/fgetflags.c b/lib/e2p/fgetflags.c
index 93e130c6..24a7166b 100644
--- a/lib/e2p/fgetflags.c
+++ b/lib/e2p/fgetflags.c
@@ -79,14 +79,26 @@ int fgetflags (const char * name, unsigned long * flags)
*flags = f;
return (save_errno);
#elif HAVE_EXT2_IOCTLS
+ struct stat buf;
int fd, r, f, save_errno = 0;

+ if (!stat(name, &buf) &&
+ !S_ISREG(buf.st_mode) && !S_ISDIR(buf.st_mode)) {
+ errno = EOPNOTSUPP;
+ return -1;
+ }
fd = open(name, OPEN_FLAGS);
if (fd == -1) {
if (errno == ELOOP || errno == ENXIO)
errno = EOPNOTSUPP;
return -1;
}
+ if (!fstat(fd, &buf) &&
+ !S_ISREG(buf.st_mode) && !S_ISDIR(buf.st_mode)) {
+ close(fd);
+ errno = EOPNOTSUPP;
+ return -1;
+ }
r = ioctl(fd, EXT2_IOC_GETFLAGS, &f);
if (r == -1) {
if (errno == ENOTTY)
diff --git a/lib/e2p/fsetflags.c b/lib/e2p/fsetflags.c
index 6455e386..d865d243 100644
--- a/lib/e2p/fsetflags.c
+++ b/lib/e2p/fsetflags.c
@@ -80,14 +80,26 @@ int fsetflags (const char * name, unsigned long flags)
int f = (int) flags;
return syscall(SYS_fsctl, name, EXT2_IOC_SETFLAGS, &f, 0);
#elif HAVE_EXT2_IOCTLS
+ struct stat buf;
int fd, r, f, save_errno = 0;

+ if (!stat(name, &buf) &&
+ !S_ISREG(buf.st_mode) && !S_ISDIR(buf.st_mode)) {
+ errno = EOPNOTSUPP;
+ return -1;
+ }
fd = open(name, OPEN_FLAGS);
if (fd == -1) {
if (errno == ELOOP || errno == ENXIO)
errno = EOPNOTSUPP;
return -1;
}
+ if (!fstat(fd, &buf) &&
+ !S_ISREG(buf.st_mode) && !S_ISDIR(buf.st_mode)) {
+ close(fd);
+ errno = EOPNOTSUPP;
+ return -1;
+ }
f = (int) flags;
r = ioctl(fd, EXT2_IOC_SETFLAGS, &f);
if (r == -1) {