linux-ext4 - Re: 答复: [PATCH v2] e2fsprogs: Check device id in advance to skip fake device name

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 11 Nov 2019 12:20:28 -0500
From:   "Theodore Y. Ts'o" <tytso@....edu>
To:     Guiyao <guiyao@...wei.com>
Cc:     "linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>,
        Mingfangsen <mingfangsen@...wei.com>,
        "ebiggers@...gle.com" <ebiggers@...gle.com>,
        "aceballos@...il.com" <aceballos@...il.com>,
        "vertaling@...vern.nl" <vertaling@...vern.nl>
Subject: Re: 答复: [PATCH v2] e2fsprogs:
 Check device id in advance to skip fake device name

On Mon, Nov 11, 2019 at 02:43:46PM +0000, Guiyao wrote:
> 
> Actually, we found some insane system administrators, they not only do something like "mount -t tmpfs /dev/sdb /tmp ", but also they do " ln -s /dev/sdb abc ", then "resize2fs abc xxx". :(

So I don't consider ourselves necessarily obligated to twist ourselves
into knots for insane system administrators.  :-)

Did you test the patch that I sent out?  It handles that case already:

% grep /dev/loop /proc/mounts
/dev/loop0 /mnt2 tmpfs rw,relatime 0 0
/dev/loop0 /mnt ext4 rw,relatime 0 0
% ln -s /dev/loop0 abc
% ./tst_ismounted abc
Device abc reports flags 11
abc is apparently in use.
abc is mounted.
abc is mounted on /mnt2.

> So we have to add the fixing code in both sides of "name matched" and "name not matched".
> 
> For the compiling issue, it's my fault in previous patch, and added the macro in a wrong line.
> 
> So, I rewrote it again, and please give more advise. Thank you in advance.

Given that I have a patch which I've already tested, and which is a
substantial clean up in terms of removing #ifdef cases and number of
lines of code:

 lib/ext2fs/ismounted.c | 39 ++++++++++++---------------------------
  1 file changed, 12 insertions(+), 27 deletions(-)

I'm inclined to stick with mine.

But here's the quick review.

>  {
>     struct mntent   *mnt;
> +#ifndef __GNU__
> +   struct stat dir_st_buf;
> +#endif  /* __GNU__ */

Lots of extra #ifdef/#ifndef is undesirable.  As it turns out, it
isn't necessary to have a separate dir_st_buf at all.

> @@ -128,13 +131,32 @@ static errcode_t check_mntent_file(const char *mtab_file, const char *file,
>     while ((mnt = getmntent (f)) != NULL) {
>         if (mnt->mnt_fsname[0] != '/')
>             continue;
> -       if (strcmp(file, mnt->mnt_fsname) == 0)
> +#ifndef __GNU__
> +       if (stat(mnt->mnt_dir, &dir_st_buf) != 0)
> +           continue;
> +#endif
> +       if (strcmp(file, mnt->mnt_fsname) == 0) {
> +#ifndef __GNU__
> +           if (file_rdev && (file_rdev != dir_st_buf.st_dev)) {

This doesn't need to be under #ifndef __GNU__.  In the GNU hurd case,
file_rdev will be zero, so the compiler will remove the if statement
for us, without needing an additional #ifndef __GNU__ test.

>         if (stat(mnt->mnt_fsname, &st_buf) == 0) {
>             if (ext2fsP_is_disk_device(st_buf.st_mode)) {
>  #ifndef __GNU__
> -               if (file_rdev && (file_rdev == st_buf.st_rdev))
> -                   break;
> +               if (file_rdev && (file_rdev == st_buf.st_rdev)) {
> +                   if (file_rdev == dir_st_buf.st_dev)
> +                       break;
> +               }
> +

The reason why this isn't necessary is because we're using stat, and
stat follows symlinks.  So when you do "ln -s /dev/sdb abc", and then
we stat abc, st_buf.st_rdev contains the device node of /dev/sbc, not
the symbolic link of abc.  So adding a check for dir_st_buf.st_dev is
not needed.

Cheers,

					- Ted