linux-ext4 - 答复: 答复: [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] [day] [month] [year] [list]

Message-ID: <005F77DB9A260B4E91664DDF22573C66E9D38A07@dggemm512-mbs.china.huawei.com>
Date:   Tue, 12 Nov 2019 14:19:43 +0000
From:   Guiyao <guiyao@...wei.com>
To:     "Theodore Y. Ts'o" <tytso@....edu>
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: 答复: 答复: [PATCH v2] e2fsprogs: Check device id in advance to skip fake device name

Hi, Ted,

Yes, I agree with you, let's remove the "#ifdef/#ifndef " code. :)

And I have tested your patch, it can work well in case 1, but cannot work in case 2.

#mount -t tmpfs /dev/sdb tmp
#mount /dev/sdb sdb

Case1:
#resize2fs /dev/sdb 7G
// it can success, and the correct sdb resized.

Case2:
#umount sdb
#ln -s /dev/sdb abc
#resize2fs abc 8G
Filesystem at abc is mounted on /root/tmp; on-line resizing required
old_desc_blocks = 2, new_desc_blocks = 2 
resize2fs: Kernel does not support online resizing

it is the reason I added some code here, and had to add "dir_st_buf ".

>         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;
> +               }

To be honest, I think the scene of case 2 is really strange.

Thank you.

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