| Message ID | 1508330318-19456-1-git-send-email-idryomov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 18, 2017 at 02:38:38PM +0200, Ilya Dryomov wrote: > Check for CAP_SYS_ADMIN before calling into the driver, similar to > blkdev_flushbuf(). This is safer and can spare a check in the driver. > > (Currently BLKROSET is overridden by md and rbd, rbd is missing the > check. md has the check, but it covers a lot more than BLKROSET.) > > Signed-off-by: Ilya Dryomov <idryomov@gmail.com> > --- > Al, this appears to go back to your "[PATCH] block ioctl cleanup", > history commit c6973580141c. 2002 was a long time ago, but still ;) > Was there a reason you made BLKFLSBUF check for CAP_SYS_ADMIN before > ->ioctl() and BLKROSET after? It was a long time ago, indeed... The funny part is, at the time there had been no ->ioctl() instances with unusual BLKROSET handling left; I really don't remember what had left to the override for those remaining and (assuming it hadn't been a plain and simple braino) the reasons for leaving the check to drivers that might eventually want to add such overrides would be in whatever discussion that had lead to leaving that override... There was a *lot* of patch series (semi)manual reordering/rebasing, so it might have easily been braindamage on conflict resolution during rebase. gendisk work had been literally hundreds of patches all over the drivers/* over the summer and autumn of 2002; I have bits and pieces of email archives from back then, but quick grep doesn't catch any discussions along those lines and they are incomplete ;-/ Anyway, a) I don't see any reason for drivers to relax the checks on BLKROSET and rbd lacking those is almost certainly a bug b) Acked-by: Al Viro <viro@zeniv.linux.org.uk> c) I can push it through vfs tree, but it would probably make more sense block one.
On Thu, Oct 19, 2017 at 2:14 AM, Al Viro <viro@zeniv.linux.org.uk> wrote: > On Wed, Oct 18, 2017 at 02:38:38PM +0200, Ilya Dryomov wrote: >> Check for CAP_SYS_ADMIN before calling into the driver, similar to >> blkdev_flushbuf(). This is safer and can spare a check in the driver. >> >> (Currently BLKROSET is overridden by md and rbd, rbd is missing the >> check. md has the check, but it covers a lot more than BLKROSET.) >> >> Signed-off-by: Ilya Dryomov <idryomov@gmail.com> >> --- >> Al, this appears to go back to your "[PATCH] block ioctl cleanup", >> history commit c6973580141c. 2002 was a long time ago, but still ;) >> Was there a reason you made BLKFLSBUF check for CAP_SYS_ADMIN before >> ->ioctl() and BLKROSET after? > > It was a long time ago, indeed... The funny part is, at the time > there had been no ->ioctl() instances with unusual BLKROSET handling > left; I really don't remember what had left to the override for > those remaining and (assuming it hadn't been a plain and simple braino) > the reasons for leaving the check to drivers that might eventually > want to add such overrides would be in whatever discussion that > had lead to leaving that override... > > There was a *lot* of patch series (semi)manual reordering/rebasing, so > it might have easily been braindamage on conflict resolution during > rebase. > > gendisk work had been literally hundreds of patches all over the > drivers/* over the summer and autumn of 2002; I have bits and pieces of > email archives from back then, but quick grep doesn't catch any > discussions along those lines and they are incomplete ;-/ > > Anyway, > a) I don't see any reason for drivers to relax the checks on > BLKROSET and rbd lacking those is almost certainly a bug > b) Acked-by: Al Viro <viro@zeniv.linux.org.uk> > c) I can push it through vfs tree, but it would probably make > more sense block one. Jens, can you pick this up for 4.15? Thanks, Ilya
On 10/24/2017 11:30 PM, Ilya Dryomov wrote: > On Thu, Oct 19, 2017 at 2:14 AM, Al Viro <viro@zeniv.linux.org.uk> wrote: >> On Wed, Oct 18, 2017 at 02:38:38PM +0200, Ilya Dryomov wrote: >>> Check for CAP_SYS_ADMIN before calling into the driver, similar to >>> blkdev_flushbuf(). This is safer and can spare a check in the driver. >>> >>> (Currently BLKROSET is overridden by md and rbd, rbd is missing the >>> check. md has the check, but it covers a lot more than BLKROSET.) >>> >>> Signed-off-by: Ilya Dryomov <idryomov@gmail.com> >>> --- >>> Al, this appears to go back to your "[PATCH] block ioctl cleanup", >>> history commit c6973580141c. 2002 was a long time ago, but still ;) >>> Was there a reason you made BLKFLSBUF check for CAP_SYS_ADMIN before >>> ->ioctl() and BLKROSET after? >> >> It was a long time ago, indeed... The funny part is, at the time >> there had been no ->ioctl() instances with unusual BLKROSET handling >> left; I really don't remember what had left to the override for >> those remaining and (assuming it hadn't been a plain and simple braino) >> the reasons for leaving the check to drivers that might eventually >> want to add such overrides would be in whatever discussion that >> had lead to leaving that override... >> >> There was a *lot* of patch series (semi)manual reordering/rebasing, so >> it might have easily been braindamage on conflict resolution during >> rebase. >> >> gendisk work had been literally hundreds of patches all over the >> drivers/* over the summer and autumn of 2002; I have bits and pieces of >> email archives from back then, but quick grep doesn't catch any >> discussions along those lines and they are incomplete ;-/ >> >> Anyway, >> a) I don't see any reason for drivers to relax the checks on >> BLKROSET and rbd lacking those is almost certainly a bug >> b) Acked-by: Al Viro <viro@zeniv.linux.org.uk> >> c) I can push it through vfs tree, but it would probably make >> more sense block one. > > Jens, can you pick this up for 4.15? Done, thanks.
diff --git a/block/ioctl.c b/block/ioctl.c index 0de02ee67eed..3f81bc50ac00 100644 --- a/block/ioctl.c +++ b/block/ioctl.c @@ -437,11 +437,12 @@ static int blkdev_roset(struct block_device *bdev, fmode_t mode, { int ret, n; + if (!capable(CAP_SYS_ADMIN)) + return -EACCES; + ret = __blkdev_driver_ioctl(bdev, mode, cmd, arg); if (!is_unrecognized_ioctl(ret)) return ret; - if (!capable(CAP_SYS_ADMIN)) - return -EACCES; if (get_user(n, (int __user *)arg)) return -EFAULT; set_device_ro(bdev, n);
Check for CAP_SYS_ADMIN before calling into the driver, similar to blkdev_flushbuf(). This is safer and can spare a check in the driver. (Currently BLKROSET is overridden by md and rbd, rbd is missing the check. md has the check, but it covers a lot more than BLKROSET.) Signed-off-by: Ilya Dryomov <idryomov@gmail.com> --- Al, this appears to go back to your "[PATCH] block ioctl cleanup", history commit c6973580141c. 2002 was a long time ago, but still ;) Was there a reason you made BLKFLSBUF check for CAP_SYS_ADMIN before ->ioctl() and BLKROSET after? --- block/ioctl.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)