sysfs_get_link doesn't handle directory symlinks (except as last arc)

[Martin von Gagern]

Binary package hint: libsysfs2

Involved packages:
pmount 0.9.17-2
libsysfs2 2.1.0-4
linux-image-2.6.27-6-generic 2.6.27-6.9

I observed this problem while trying to use pmount. Relevant lines only:
$ pmount /dev/sdb1
Error: device /dev/sdb1 is not removable
$ pmount --debug /dev/sdb1
device_removable: could not find a sysfs device for /dev/sdb1
$ sudo ltrace -s 1024 pmount /dev/sdb1
sysfs_get_link("/sys/block/sdb/device", "/2:0:0:0", 1024) = 0
sysfs_open_device_path("/2:0:0:0") = 0
$ readlink /sys/block/sdb/device
../../../2:0:0:0
$ readlink /sys/block/sdb
../devices/pci0000:00/0000:00:1d.0/usb1/1-1/1-1:1.0/host2/target2:0:0/2:0:0:0/block/sdb
$ ( cd /sys/block/sdb/device; pwd -P; )
/sys/devices/pci0000:00/0000:00:1d.0/usb1/1-1/1-1:1.0/host2/target2:0:0/2:0:0:0

So the symlink does point to a valid device directory when all symlinks in the path are considered or, alternatively, when .. is treated like any subdirectroy and not canceled with the preceding arc. The sysfs_get_link function, however, cancels arcs without checking for symlinks. This leads to an incorrect absolute path, and thus causes pmount to fail.

I'm not perfectly sure whether sysfs_get_link is intended to deal with such issues. Looking at libsysfs.txt shipped with the sysfsutils sources I would guess so, though. And as sysfsutils is a probably security relevant piece of code, I'd rather play it safe, and have the library perform some extra checks instead of relying on applications to take care of such details. On the other hand, if this issue were to take longer to get fixed, pmount could probably work around it by simply not calling sysfs_get_link at all, but instead have the OS deal with symlinks.

Looking at the code of sysfs_get_link from sysfs_utils.c, it looks like this issue might cause more severe problems. This is the part where the path stripping actually happens, having detected the link path (d) to start with "..":

  while (*d == '/' || *d == '.') {
        if (*d == '/')
        slashes++;
        d++;
  }
  d--;
  s = &devdir[strlen(devdir)-1];
  while (s != NULL && count != (slashes+1)) {
        s--;
        if (*s == '/')
              count++;
  }
  safestrcpymax(s, d, (SYSFS_PATH_MAX-strlen(devdir)));

As you can see, the code moves backwards in the directory path. However, the beginning of string comparison seems to me completely bogus. So when there aren't enough slashes, this code will iterate before the beginning of the string. I guess it should be something like "s != devdir" instead of "s != NULL".

The assumption that there will be only arcs of the form ".." involved, no "." or "...." isn't completely justified either, although it should for the most part hold in sysfs.

I guess one could also modify this loop to replace every '/' by '\0' and check the thus truncated path recursively. I'm not perfectly sure whether one would have to take special care in order to prevent infinite loops; on sane file systems I wouldn't think so.

ProblemType: Bug
Architecture: i386
Dependencies:
 libgcc1 1:4.3.2-1ubuntu9
 gcc-4.3-base 4.3.2-1ubuntu9
 findutils 4.4.0-2ubuntu3
 libc6 2.8~20080505-0ubuntu7
DistroRelease: Ubuntu 8.10
Package: libsysfs2 2.1.0-4
ProcEnviron:
 LC_CTYPE=de_DE.utf8
 PATH=/home/username/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games
 LANG=de_DE.utf8
 LC_MESSAGES=C
 SHELL=/bin/bash
SourcePackage: sysfsutils
Uname: Linux 2.6.27-6-generic i686

[Vincent Fourmond (fourmond)]

(I'm the current pmount maintainer)

sysfs is deprecated, see in Documentation/sysfs-rules.txt:

- Do not use libsysfs
  It makes assumptions about sysfs which are not true. Its API does not
  offer any abstraction, it exposes all the kernel driver-core
  implementation details in its own API. Therefore it is not better than
  reading directories and opening the files yourself.
  Also, it is not actively maintained, in the sense of reflecting the
  current kernel development. The goal of providing a stable interface
  to sysfs has failed; it causes more problems than it solves. It
  violates many of the rules in this document.

  I plan to rewrite some bits of pmount to drop the dependency on libsysfs, but that won't happen before a while. Until then, there will be no fix for that, unfortunately...

[Piotr Kęplicz (keplicz)]

As reported in bug 346704, pmount 0.9.19 in Debian correctly recognizes removable devices.