On Wed 20-03-19 12:21:29, Dongli Zhang wrote:
> On 3/19/19 5:41 PM, Jan Kara wrote:
> > On Mon 18-03-19 20:27:02, Dongli Zhang wrote:
> >> Hi Jan,
> >>
> >> Indeed there is another issue implicitly reported by below console output from
> >> syzkaller:
> >>
> >> [ 245.524424][ T9455] loop_reread_partitions: partition scan of loop0 () failed
> >> (rc=-13)
> >> [ 245.563340][ T9499] kasan: CONFIG_KASAN_INLINE enabled
> >> [ 245.576412][ T9489] __loop_clr_fd: partition scan of loop0 failed (rc=-13)
> >> [ 245.581275][ T9499] kasan: GPF could be caused by NULL-ptr deref or user
> >> memory access
> >> [ 245.602596][ T9499] general protection fault: 0000 [#1] PREEMPT SMP KASAN
> >>
> >> I think rc=-13 is because of below code at line 168:
> >>
> >> 162 int __blkdev_reread_part(struct block_device *bdev)
> >> 163 {
> >> 164 struct gendisk *disk = bdev->bd_disk;
> >> 165
> >> 166 if (!disk_part_scan_enabled(disk) || bdev != bdev->bd_contains)
> >> 167 return -EINVAL;
> >> 168 if (!capable(CAP_SYS_ADMIN))
> >> 169 return -EACCES;
> >> 170
> >> 171 lockdep_assert_held(&bdev->bd_mutex);
> >> 172
> >> 173 return rescan_partitions(disk, bdev);
> >> 174 }
> >> 175 EXPORT_SYMBOL(__blkdev_reread_part);
> >>
> >> I can reproduce this by 'chown username /dev/loop0' on my test machine.
> >>
> >> Taking 'losetup -d /dev/loop0' as sample, as /dev/loop0 belongs to my username,
> >> I am able to detach the loop without 'su'.
> >>
> >> However, because of above line 168, the partition scan would fail.
> >>
> >> Should we always assume the user should have admin privilege to detach
> >> the loop and this is not a bug?
> >
> > Firstly, __blkdev_reread_part() is used for all devices so we have to be
> > *very* careful when we relax the permission checks there.
> >
> > Secondly, I'm not convinced it is always safe to allow non-priviledged user
> > to force repartitioning of a device. That exposes the whole partition table
> > parsing code to non-priviledged user and thus increases possible attack
> > surface.
> >
> > But in this specific case we call __blkdev_reread_part() only to tear down
> > partitions. So in this specific case calling it by unpriviledged user is
> > fine plus leaving stale partitions behind is certainly not nice. What we
> > could do is:
> >
> > Export drop_partitions() functionality as a function like
> > __blkdev_drop_part() that would call drop_partitions(bdev->bd_disk, bdev).
> > It would expect (and assert) that &bdev->bd_mutex is already locked. It
> > should probably also replicate the check:
> >
> > if (!disk_part_scan_enabled(disk) || bdev != bdev->bd_contains)
> > return -EINVAL;
> >
> > from __blkdev_reread_part(). And we can call this from __loop_clr_fd().
> >
> > Then we can just unexport __blkdev_reread_part() (since only loop.c is
> > using it) and fold it inside blkdev_reread_part().
> >
> > What do you think?
> >
> > Honza
> >
>
> The idea is to duplicate a new caller of drop_partitions() which is
> specifically used by loop. I think it works. It is safe as well as the
> functionality is limited within loop.
>
> However, perhaps we should not regard this as a bug? Below is the sample
> to reproduce this issue:
>
> $ dd if=/dev/zero of=tmp.raw bs=1M count=100 oflag=direct
>
> $ parted tmp.raw --script mklabel msdos mkpart primary 0% 50% mkpart primary 50%
> 100%
>
> $ sudo chown zhang /dev/loop0
>
> $ losetup -P /dev/loop0 tmp.raw
>
> $ ls -l /dev | grep loop0
> brw-rw---- 1 zhang disk 7, 0 Mar 20 11:42 loop0 --> user (zhang)
> brw-rw---- 1 root disk 259, 0 Mar 20 11:42 loop0p1 --> root
> brw-rw---- 1 root disk 259, 1 Mar 20 11:42 loop0p2 --> root
>
> $ losetup -d /dev/loop
>
> From above case, /dev/loop0 is owned by user (zhang), while the partitions are
> owned by root.
>
> Should the detach of loop owned by user also unmaps all related partitions owned
> by root?
>
> Perhaps we should assume the '-P' option is always used by root?
Yes, that's another option. And since I'm not aware of any user reports of
the problem you've found I'd just say that non-root users don't use loop
devices with partitions.
> This is similar to the fact that the administrator should always use '-P'
> in order to enforce partscan when the loop is detached. Otherwise, the
> partitions belong to the loop would remain after 'losetup -d'.
Yeah. That seems like another slight catch for the users. Honestly, I don't
feel either of these issues is serious enough that I'd go and fix them. But
if someone wanted to spend the time with them, I won't object :).
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
