| Message ID | 9dcadf7a-b12a-c977-2de2-222e20f0cebe@huawei.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] resize2fs: resize2fs disk hardlinks will be error | expand |
friendly ping.. 在 2022/1/4 22:23, zhanchengbin 写道: > Resize2fs disk hardlinks which mounting after the same name as tmpfs > filesystem it will be error. The items in /proc/mounts are traversed, > when you get to tmpfs,file!=mnt->mnt_fsname, therefore, the > stat(mnt->mnt_fsname, &st_buf) branch is used, however, the values of > file_rdev and st_buf.st_rdev are the same. As a result, the system > mistakenly considers that disk is mounted to /root/tmp. As a result > , resize2fs fails. > > example: > dev_name="/dev/sdc" (ps: a disk in you self) > mkdir /root/tmp > mkdir /root/mnt > mkfs.ext4 -F -b 1024 -E "resize=10000000" "${dev_name}" 32768 > mount -t tmpfs "${dev_name}" /root/tmp > mount "${dev_name}" /root/mnt > ln "${dev_name}" "${dev_name}"-ln > resize2fs "${dev_name}"-ln 6G > > Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com> > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com> > > --- > lib/ext2fs/ismounted.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/lib/ext2fs/ismounted.c b/lib/ext2fs/ismounted.c > index aee7d726..463a82a6 100644 > --- a/lib/ext2fs/ismounted.c > +++ b/lib/ext2fs/ismounted.c > @@ -98,6 +98,7 @@ static errcode_t check_mntent_file(const char > *mtab_file, const char *file, > { > struct mntent *mnt; > struct stat st_buf; > + struct stat dir_st_buf; > errcode_t retval = 0; > dev_t file_dev=0, file_rdev=0; > ino_t file_ino=0; > @@ -144,8 +145,12 @@ static errcode_t check_mntent_file(const char > *mtab_file, const char *file, > 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 (stat(mnt->mnt_dir, &dir_st_buf) != 0) > + continue; > + if (file_rdev == dir_st_buf.st_dev) > + break; > + } > if (check_loop_mounted(mnt->mnt_fsname, > st_buf.st_rdev, file_dev, > file_ino) == 1)
On Tue, Jan 04, 2022 at 10:23:52PM +0800, zhanchengbin wrote: > Resize2fs disk hardlinks which mounting after the same name as tmpfs > filesystem it will be error. The items in /proc/mounts are traversed, > when you get to tmpfs,file!=mnt->mnt_fsname, therefore, the > stat(mnt->mnt_fsname, &st_buf) branch is used, however, the values of > file_rdev and st_buf.st_rdev are the same. As a result, the system > mistakenly considers that disk is mounted to /root/tmp. As a result > , resize2fs fails. Apologies for the delay in getting to this patch. The original patch[1] was corrupted (looks like you used Mozilla Thunderbird as your Mail User Agent, which line-wrapped the patch and thus confused patchwork[2] as well making it impossible for b4 and git am to handle the patch). [1] https://lore.kernel.org/all/9dcadf7a-b12a-c977-2de2-222e20f0cebe@huawei.com/ [2] http://patchwork.ozlabs.org/project/linux-ext4/patch/9dcadf7a-b12a-c977-2de2-222e20f0cebe@huawei.com/ I also rewrite the commit description to make it more clear: libext2fs: add extra checks to ext2fs_check_mount_point() A pseudo-filesystem, such as tmpfs, can have anything at all in its mnt_fsname entry. Normally, it is just "tmpfs", like this: tmpfs /tmp tmpfs rw,relatime,inode64 0 0 ^^^^^ but in a pathological or malicious case, a system administrator can specify a block device as its mnt_fsname which is the same as some other block device. For example: /dev/loop0 /tmp/test-tmpfs tmpfs rw,relatime,inode64 0 0 ^^^^^^^^^^ /dev/loop0 /tmp/test-mnt ext4 rw,relatime 0 0 In this case, ext2fs_check_mount_point() may erroneously return that the mountpoint for the file system on /dev/loop0 is mounted on /tmp/test-tmpfs, instead of the correct /tmp/test-mnt. This causes problems for resize2fs, since in order to do an online resize, it needs to open the directory where the file system is mounted, and trigger the online resize ioctl. If it opens the incorrect directory, then resize2fs will fail. So we need to add some additional checking to make sure that directory's st_dev matches the block device's st_rdev field. An example shell script which reproduces the problem fixed by this commit is as follows: loop_file=/tmp/foo.img tmpfs_dir=/tmp/test-tmpfs mnt_dir=/tmp/test-mnt mkdir -p $tmpfs_dir $mnt_dir dd if=/dev/zero of=$loop_file bs=1k count=65536 test_dev=$(losetup --show -f $loop_file) mke2fs -t ext4 -F -b 1024 $test_dev 32768 mount -t tmpfs $test_dev $tmpfs_dir # create the evil /proc/mounts entry mount -t ext4 $test_dev $mnt_dir ln -f ${test_dev} ${test_dev}-ln resize2fs ${test_dev}-ln [ Fixed up the corrupted patch and rewrote the commit description to be more clear -- tytso ] Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com> Signed-off-by: Theodore Ts'o <tytso@mit.edu> As you can see, the best commit description describes *why* a particular change is needed, and gives the background so the reader can understand what problem is being fixed. The one-line change makes it clear that the change is to libext2fs's ismounted.c, and *not* to resize2fs, although you were making this bug to fix resize2fs after a system administrator did something non-standard and/or malicious. Also note how I rewrote the reproducer to be simpler and more portable. Cheers, - Ted
On Jan 17, 2023, at 10:05 PM, Theodore Ts'o <tytso@mit.edu> wrote: > > On Tue, Jan 04, 2022 at 10:23:52PM +0800, zhanchengbin wrote: >> Resize2fs disk hardlinks which mounting after the same name as tmpfs >> filesystem it will be error. The items in /proc/mounts are traversed, >> when you get to tmpfs,file!=mnt->mnt_fsname, therefore, the >> stat(mnt->mnt_fsname, &st_buf) branch is used, however, the values of >> file_rdev and st_buf.st_rdev are the same. As a result, the system >> mistakenly considers that disk is mounted to /root/tmp. As a result >> , resize2fs fails. Rather than doing ever more complex processing of /proc/mounts, it would make more sense to have resize2fs take the *mountpoint* of the filesystem to be resized, and then run the ioctl directly on the given path. That avoids the need to process /proc/mounts at all, and unambiguously specifies which filesystem should be resized. For compatibility, resize2fs would need to accept either the device or mountpoint (really any directory in the filesystem would work), and it could check either the mnt_fsname or mnt_dir matches. Cheers, Andreas > Apologies for the delay in getting to this patch. The original > patch[1] was corrupted (looks like you used Mozilla Thunderbird as > your Mail User Agent, which line-wrapped the patch and thus confused > patchwork[2] as well making it impossible for b4 and git am to handle > the patch). > > [1] https://lore.kernel.org/all/9dcadf7a-b12a-c977-2de2-222e20f0cebe@huawei.com/ > [2] http://patchwork.ozlabs.org/project/linux-ext4/patch/9dcadf7a-b12a-c977-2de2-222e20f0cebe@huawei.com/ > > I also rewrite the commit description to make it more clear: > > libext2fs: add extra checks to ext2fs_check_mount_point() > > A pseudo-filesystem, such as tmpfs, can have anything at all in its > mnt_fsname entry. Normally, it is just "tmpfs", like this: > > tmpfs /tmp tmpfs rw,relatime,inode64 0 0 > ^^^^^ > > but in a pathological or malicious case, a system administrator can > specify a block device as its mnt_fsname which is the same as some > other block device. For example: > > /dev/loop0 /tmp/test-tmpfs tmpfs rw,relatime,inode64 0 0 > ^^^^^^^^^^ > /dev/loop0 /tmp/test-mnt ext4 rw,relatime 0 0 > > In this case, ext2fs_check_mount_point() may erroneously return > that the mountpoint for the file system on /dev/loop0 is mounted on > /tmp/test-tmpfs, instead of the correct /tmp/test-mnt. This causes > problems for resize2fs, since in order to do an online resize, it > needs to open the directory where the file system is mounted, and > trigger the online resize ioctl. If it opens the incorrect directory, > then resize2fs will fail. > > So we need to add some additional checking to make sure that > directory's st_dev matches the block device's st_rdev field. > > An example shell script which reproduces the problem fixed by this > commit is as follows: > > loop_file=/tmp/foo.img > tmpfs_dir=/tmp/test-tmpfs > mnt_dir=/tmp/test-mnt > > mkdir -p $tmpfs_dir $mnt_dir > dd if=/dev/zero of=$loop_file bs=1k count=65536 > test_dev=$(losetup --show -f $loop_file) > mke2fs -t ext4 -F -b 1024 $test_dev 32768 > mount -t tmpfs $test_dev $tmpfs_dir # create the evil /proc/mounts entry > mount -t ext4 $test_dev $mnt_dir > ln -f ${test_dev} ${test_dev}-ln > resize2fs ${test_dev}-ln > > [ Fixed up the corrupted patch and rewrote the commit description to > be more clear -- tytso ] > > Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com> > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com> > Signed-off-by: Theodore Ts'o <tytso@mit.edu> > > As you can see, the best commit description describes *why* a > particular change is needed, and gives the background so the reader > can understand what problem is being fixed. The one-line change makes > it clear that the change is to libext2fs's ismounted.c, and *not* to > resize2fs, although you were making this bug to fix resize2fs after a > system administrator did something non-standard and/or malicious. > > Also note how I rewrote the reproducer to be simpler and more > portable. > > Cheers, > > - Ted Cheers, Andreas
diff --git a/lib/ext2fs/ismounted.c b/lib/ext2fs/ismounted.c index aee7d726..463a82a6 100644 --- a/lib/ext2fs/ismounted.c +++ b/lib/ext2fs/ismounted.c @@ -98,6 +98,7 @@ static errcode_t check_mntent_file(const char *mtab_file, const char *file, { struct mntent *mnt; struct stat st_buf; + struct stat dir_st_buf; errcode_t retval = 0; dev_t file_dev=0, file_rdev=0;
Resize2fs disk hardlinks which mounting after the same name as tmpfs filesystem it will be error. The items in /proc/mounts are traversed, when you get to tmpfs,file!=mnt->mnt_fsname, therefore, the stat(mnt->mnt_fsname, &st_buf) branch is used, however, the values of file_rdev and st_buf.st_rdev are the same. As a result, the system mistakenly considers that disk is mounted to /root/tmp. As a result , resize2fs fails. example: dev_name="/dev/sdc" (ps: a disk in you self) mkdir /root/tmp mkdir /root/mnt mkfs.ext4 -F -b 1024 -E "resize=10000000" "${dev_name}" 32768 mount -t tmpfs "${dev_name}" /root/tmp mount "${dev_name}" /root/mnt ln "${dev_name}" "${dev_name}"-ln resize2fs "${dev_name}"-ln 6G Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com> --- lib/ext2fs/ismounted.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) ino_t file_ino=0; @@ -144,8 +145,12 @@ static errcode_t check_mntent_file(const char *mtab_file, const char *file, 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 (stat(mnt->mnt_dir, &dir_st_buf) != 0) + continue; + if (file_rdev == dir_st_buf.st_dev) + break; + } if (check_loop_mounted(mnt->mnt_fsname, st_buf.st_rdev, file_dev, file_ino) == 1)