From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754030AbcA2JWg (ORCPT ); Fri, 29 Jan 2016 04:22:36 -0500 Received: from mail-wm0-f54.google.com ([74.125.82.54]:37757 "EHLO mail-wm0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753721AbcA2JWa (ORCPT ); Fri, 29 Jan 2016 04:22:30 -0500 MIME-Version: 1.0 In-Reply-To: References: From: Dmitry Vyukov Date: Fri, 29 Jan 2016 10:22:09 +0100 Message-ID: Subject: Re: [PATCH] floppy: fix lock_fdc() signal handling To: syzkaller Cc: Jens Axboe , NeilBrown , Takashi Iwai , Hannes Reinecke , Rasmus Villemoes , LKML , Kostya Serebryany , Alexander Potapenko , Sasha Levin Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 28, 2016 at 11:43 AM, Jiri Kosina wrote: > On Thu, 28 Jan 2016, Dmitry Vyukov wrote: > >> I have not seen any /dev/fd0 related crashes over night. Previously I >> seen tons of them, so I guess this bug is fixed. >> Please send this upstream. >> Thanks > > Thanks a lot for testing, Dmitry. > > Jens, here is the patch with proper changelog. Could you please apply it > to your tree? Or would you prefer a pull request with this single patch in > it? > Thanks. Done Replaced the old patch with this one. Thanks for a quick fix > From: Jiri Kosina > Subject: [PATCH] floppy: fix lock_fdc() signal handling > > floppy_revalidate() doesn't perform any error handling on lock_fdc() > result. lock_fdc() might actually be interrupted by a signal (it waits for > fdc becoming non-busy interruptibly). In such case, floppy_revalidate() > proceeds as if it had claimed the lock, but it fact it doesn't. > > In case of multiple threads trying to open("/dev/fdX"), this leads to > serious corruptions all over the place, because all of a sudden there is > no critical section protection (that'd otherwise be guaranteed by locked > fd) whatsoever. > > While at this, fix the fact that the 'interruptible' parameter to > lock_fdc() doesn't make any sense whatsoever, because we always wait > interruptibly anyway. > > Most of the lock_fdc() callsites do properly handle error (and propagate > EINTR), but floppy_revalidate() and floppy_check_events() don't. Fix this. > > Spotted by 'syzkaller' tool. > > Reported-by: Dmitry Vyukov > Tested-by: Dmitry Vyukov > Signed-off-by: Jiri Kosina > --- > drivers/block/floppy.c | 33 ++++++++++++++++++--------------- > 1 file changed, 18 insertions(+), 15 deletions(-) > > diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c > index 9e25120..b206115 100644 > --- a/drivers/block/floppy.c > +++ b/drivers/block/floppy.c > @@ -866,7 +866,7 @@ static void set_fdc(int drive) > } > > /* locks the driver */ > -static int lock_fdc(int drive, bool interruptible) > +static int lock_fdc(int drive) > { > if (WARN(atomic_read(&usage_count) == 0, > "Trying to lock fdc while usage count=0\n")) > @@ -2173,7 +2173,7 @@ static int do_format(int drive, struct format_descr *tmp_format_req) > { > int ret; > > - if (lock_fdc(drive, true)) > + if (lock_fdc(drive)) > return -EINTR; > > set_floppy(drive); > @@ -2960,7 +2960,7 @@ static int user_reset_fdc(int drive, int arg, bool interruptible) > { > int ret; > > - if (lock_fdc(drive, interruptible)) > + if (lock_fdc(drive)) > return -EINTR; > > if (arg == FD_RESET_ALWAYS) > @@ -3243,7 +3243,7 @@ static int set_geometry(unsigned int cmd, struct floppy_struct *g, > if (!capable(CAP_SYS_ADMIN)) > return -EPERM; > mutex_lock(&open_lock); > - if (lock_fdc(drive, true)) { > + if (lock_fdc(drive)) { > mutex_unlock(&open_lock); > return -EINTR; > } > @@ -3263,7 +3263,7 @@ static int set_geometry(unsigned int cmd, struct floppy_struct *g, > } else { > int oldStretch; > > - if (lock_fdc(drive, true)) > + if (lock_fdc(drive)) > return -EINTR; > if (cmd != FDDEFPRM) { > /* notice a disk change immediately, else > @@ -3349,7 +3349,7 @@ static int get_floppy_geometry(int drive, int type, struct floppy_struct **g) > if (type) > *g = &floppy_type[type]; > else { > - if (lock_fdc(drive, false)) > + if (lock_fdc(drive)) > return -EINTR; > if (poll_drive(false, 0) == -EINTR) > return -EINTR; > @@ -3433,7 +3433,7 @@ static int fd_locked_ioctl(struct block_device *bdev, fmode_t mode, unsigned int > if (UDRS->fd_ref != 1) > /* somebody else has this drive open */ > return -EBUSY; > - if (lock_fdc(drive, true)) > + if (lock_fdc(drive)) > return -EINTR; > > /* do the actual eject. Fails on > @@ -3445,7 +3445,7 @@ static int fd_locked_ioctl(struct block_device *bdev, fmode_t mode, unsigned int > process_fd_request(); > return ret; > case FDCLRPRM: > - if (lock_fdc(drive, true)) > + if (lock_fdc(drive)) > return -EINTR; > current_type[drive] = NULL; > floppy_sizes[drive] = MAX_DISK_SIZE << 1; > @@ -3467,7 +3467,7 @@ static int fd_locked_ioctl(struct block_device *bdev, fmode_t mode, unsigned int > UDP->flags &= ~FTD_MSG; > return 0; > case FDFMTBEG: > - if (lock_fdc(drive, true)) > + if (lock_fdc(drive)) > return -EINTR; > if (poll_drive(true, FD_RAW_NEED_DISK) == -EINTR) > return -EINTR; > @@ -3484,7 +3484,7 @@ static int fd_locked_ioctl(struct block_device *bdev, fmode_t mode, unsigned int > return do_format(drive, &inparam.f); > case FDFMTEND: > case FDFLUSH: > - if (lock_fdc(drive, true)) > + if (lock_fdc(drive)) > return -EINTR; > return invalidate_drive(bdev); > case FDSETEMSGTRESH: > @@ -3507,7 +3507,7 @@ static int fd_locked_ioctl(struct block_device *bdev, fmode_t mode, unsigned int > outparam = UDP; > break; > case FDPOLLDRVSTAT: > - if (lock_fdc(drive, true)) > + if (lock_fdc(drive)) > return -EINTR; > if (poll_drive(true, FD_RAW_NEED_DISK) == -EINTR) > return -EINTR; > @@ -3530,7 +3530,7 @@ static int fd_locked_ioctl(struct block_device *bdev, fmode_t mode, unsigned int > case FDRAWCMD: > if (type) > return -EINVAL; > - if (lock_fdc(drive, true)) > + if (lock_fdc(drive)) > return -EINTR; > set_floppy(drive); > i = raw_cmd_ioctl(cmd, (void __user *)param); > @@ -3539,7 +3539,7 @@ static int fd_locked_ioctl(struct block_device *bdev, fmode_t mode, unsigned int > process_fd_request(); > return i; > case FDTWADDLE: > - if (lock_fdc(drive, true)) > + if (lock_fdc(drive)) > return -EINTR; > twaddle(); > process_fd_request(); > @@ -3748,7 +3748,8 @@ static unsigned int floppy_check_events(struct gendisk *disk, > return DISK_EVENT_MEDIA_CHANGE; > > if (time_after(jiffies, UDRS->last_checked + UDP->checkfreq)) { > - lock_fdc(drive, false); > + if (lock_fdc(drive)) > + return -EINTR; > poll_drive(false, 0); > process_fd_request(); > } > @@ -3847,7 +3848,9 @@ static int floppy_revalidate(struct gendisk *disk) > "VFS: revalidate called on non-open device.\n")) > return -EFAULT; > > - lock_fdc(drive, false); > + res = lock_fdc(drive); > + if (res) > + return res; > cf = (test_bit(FD_DISK_CHANGED_BIT, &UDRS->flags) || > test_bit(FD_VERIFY_BIT, &UDRS->flags)); > if (!(cf || test_bit(drive, &fake_change) || drive_no_geom(drive))) {