Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1465540 > unrolled thread

[PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only open

Started byGreg Kroah-Hartman <gregkh@linuxfoundation.org>
First post2016-08-18 16:30 +0200
Last post2016-08-25 17:20 +0200
Articles 7 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only open Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-08-18 16:30 +0200
    Re: [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only  open Mark Hounschell <markh@compro.net> - 2016-08-24 15:40 +0200
      Re: [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only  open Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-08-24 22:30 +0200
        Re: [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only  open Jiri Kosina <jikos@kernel.org> - 2016-08-24 23:20 +0200
          Re: [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only  open Mark Hounschell <markh@compro.net> - 2016-08-25 15:10 +0200
            Re: [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only  open Mark Hounschell <markh@compro.net> - 2016-08-25 17:00 +0200
            Re: [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only  open Jiri Kosina <jikos@kernel.org> - 2016-08-25 17:20 +0200

#1465540 — [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only open

FromGreg Kroah-Hartman <gregkh@linuxfoundation.org>
Date2016-08-18 16:30 +0200
Subject[PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only open
Message-ID<s7wuv-WK-89@gated-at.bofh.it>
4.7-stable review patch.  If anyone has any objections, please let me know.

------------------

From: Jiri Kosina <jkosina@suse.cz>

commit ff06db1efb2ad6db06eb5b99b88a0c15a9cc9b0e upstream.

Commit 09954bad4 ("floppy: refactor open() flags handling"), as a
side-effect, causes open(/dev/fdX, O_ACCMODE) to fail. It turns out that
this is being used setfdprm userspace for ioctl-only open().

Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE)
modes, while still keeping the original O_NDELAY bug fixed.

Reported-by: Wim Osterholt <wim@djo.tudelft.nl>
Tested-by: Wim Osterholt <wim@djo.tudelft.nl>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Jens Axboe <axboe@fb.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 drivers/block/floppy.c |   21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -3663,11 +3663,6 @@ static int floppy_open(struct block_devi
 
 	opened_bdev[drive] = bdev;
 
-	if (!(mode & (FMODE_READ|FMODE_WRITE))) {
-		res = -EINVAL;
-		goto out;
-	}
-
 	res = -ENXIO;
 
 	if (!floppy_track_buffer) {
@@ -3711,13 +3706,15 @@ static int floppy_open(struct block_devi
 	if (UFDCS->rawcmd == 1)
 		UFDCS->rawcmd = 2;
 
-	UDRS->last_checked = 0;
-	clear_bit(FD_OPEN_SHOULD_FAIL_BIT, &UDRS->flags);
-	check_disk_change(bdev);
-	if (test_bit(FD_DISK_CHANGED_BIT, &UDRS->flags))
-		goto out;
-	if (test_bit(FD_OPEN_SHOULD_FAIL_BIT, &UDRS->flags))
-		goto out;
+	if (mode & (FMODE_READ|FMODE_WRITE)) {
+		UDRS->last_checked = 0;
+		clear_bit(FD_OPEN_SHOULD_FAIL_BIT, &UDRS->flags);
+		check_disk_change(bdev);
+		if (test_bit(FD_DISK_CHANGED_BIT, &UDRS->flags))
+			goto out;
+		if (test_bit(FD_OPEN_SHOULD_FAIL_BIT, &UDRS->flags))
+			goto out;
+	}
 
 	res = -EROFS;

[toc] | [next] | [standalone]

#1469446 — Re: [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only open

FromMark Hounschell <markh@compro.net>
Date2016-08-24 15:40 +0200
SubjectRe: [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only open
Message-ID<s9Gzp-2Q5-51@gated-at.bofh.it>
In reply to#1465540
On 08/18/2016 09:59 AM, Greg Kroah-Hartman wrote:
> 4.7-stable review patch.  If anyone has any objections, please let me know.
>
> ------------------
>
> From: Jiri Kosina <jkosina@suse.cz>
>
> commit ff06db1efb2ad6db06eb5b99b88a0c15a9cc9b0e upstream.
>
> Commit 09954bad4 ("floppy: refactor open() flags handling"), as a
> side-effect, causes open(/dev/fdX, O_ACCMODE) to fail. It turns out that
> this is being used setfdprm userspace for ioctl-only open().
>
> Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE)
> modes, while still keeping the original O_NDELAY bug fixed.
>
> Reported-by: Wim Osterholt <wim@djo.tudelft.nl>
> Tested-by: Wim Osterholt <wim@djo.tudelft.nl>
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> Signed-off-by: Jens Axboe <axboe@fb.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> ---
>  drivers/block/floppy.c |   21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
>
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -3663,11 +3663,6 @@ static int floppy_open(struct block_devi
>
>  	opened_bdev[drive] = bdev;
>
> -	if (!(mode & (FMODE_READ|FMODE_WRITE))) {
> -		res = -EINVAL;
> -		goto out;
> -	}
> -
>  	res = -ENXIO;
>
>  	if (!floppy_track_buffer) {
> @@ -3711,13 +3706,15 @@ static int floppy_open(struct block_devi
>  	if (UFDCS->rawcmd == 1)
>  		UFDCS->rawcmd = 2;
>
> -	UDRS->last_checked = 0;
> -	clear_bit(FD_OPEN_SHOULD_FAIL_BIT, &UDRS->flags);
> -	check_disk_change(bdev);
> -	if (test_bit(FD_DISK_CHANGED_BIT, &UDRS->flags))
> -		goto out;
> -	if (test_bit(FD_OPEN_SHOULD_FAIL_BIT, &UDRS->flags))
> -		goto out;
> +	if (mode & (FMODE_READ|FMODE_WRITE)) {
> +		UDRS->last_checked = 0;
> +		clear_bit(FD_OPEN_SHOULD_FAIL_BIT, &UDRS->flags);
> +		check_disk_change(bdev);
> +		if (test_bit(FD_DISK_CHANGED_BIT, &UDRS->flags))
> +			goto out;
> +		if (test_bit(FD_OPEN_SHOULD_FAIL_BIT, &UDRS->flags))
> +			goto out;
> +	}
>
>  	res = -EROFS;
>
>
>
>

I have a problem with this patch. It only fixes one of the regressions 
caused by the original change to the floppy driver. It does not address 
the user land breakage of removing the NODELAY flag checks.

Thanks
Mark

[toc] | [prev] | [next] | [standalone]

#1469691 — Re: [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only open

FromGreg Kroah-Hartman <gregkh@linuxfoundation.org>
Date2016-08-24 22:30 +0200
SubjectRe: [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only open
Message-ID<s9MY9-7ov-3@gated-at.bofh.it>
In reply to#1469446
On Wed, Aug 24, 2016 at 09:34:44AM -0400, Mark Hounschell wrote:
> On 08/18/2016 09:59 AM, Greg Kroah-Hartman wrote:
> > 4.7-stable review patch.  If anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: Jiri Kosina <jkosina@suse.cz>
> > 
> > commit ff06db1efb2ad6db06eb5b99b88a0c15a9cc9b0e upstream.
> > 
> > Commit 09954bad4 ("floppy: refactor open() flags handling"), as a
> > side-effect, causes open(/dev/fdX, O_ACCMODE) to fail. It turns out that
> > this is being used setfdprm userspace for ioctl-only open().
> > 
> > Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE)
> > modes, while still keeping the original O_NDELAY bug fixed.
> > 
> > Reported-by: Wim Osterholt <wim@djo.tudelft.nl>
> > Tested-by: Wim Osterholt <wim@djo.tudelft.nl>
> > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> > Signed-off-by: Jens Axboe <axboe@fb.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > 
> > ---
> >  drivers/block/floppy.c |   21 +++++++++------------
> >  1 file changed, 9 insertions(+), 12 deletions(-)
> > 
> > --- a/drivers/block/floppy.c
> > +++ b/drivers/block/floppy.c
> > @@ -3663,11 +3663,6 @@ static int floppy_open(struct block_devi
> > 
> >  	opened_bdev[drive] = bdev;
> > 
> > -	if (!(mode & (FMODE_READ|FMODE_WRITE))) {
> > -		res = -EINVAL;
> > -		goto out;
> > -	}
> > -
> >  	res = -ENXIO;
> > 
> >  	if (!floppy_track_buffer) {
> > @@ -3711,13 +3706,15 @@ static int floppy_open(struct block_devi
> >  	if (UFDCS->rawcmd == 1)
> >  		UFDCS->rawcmd = 2;
> > 
> > -	UDRS->last_checked = 0;
> > -	clear_bit(FD_OPEN_SHOULD_FAIL_BIT, &UDRS->flags);
> > -	check_disk_change(bdev);
> > -	if (test_bit(FD_DISK_CHANGED_BIT, &UDRS->flags))
> > -		goto out;
> > -	if (test_bit(FD_OPEN_SHOULD_FAIL_BIT, &UDRS->flags))
> > -		goto out;
> > +	if (mode & (FMODE_READ|FMODE_WRITE)) {
> > +		UDRS->last_checked = 0;
> > +		clear_bit(FD_OPEN_SHOULD_FAIL_BIT, &UDRS->flags);
> > +		check_disk_change(bdev);
> > +		if (test_bit(FD_DISK_CHANGED_BIT, &UDRS->flags))
> > +			goto out;
> > +		if (test_bit(FD_OPEN_SHOULD_FAIL_BIT, &UDRS->flags))
> > +			goto out;
> > +	}
> > 
> >  	res = -EROFS;
> > 
> > 
> > 
> > 
> 
> I have a problem with this patch. It only fixes one of the regressions
> caused by the original change to the floppy driver. It does not address the
> user land breakage of removing the NODELAY flag checks.

Does the same problem also happen in Linus's tree?  If not, any hints on
the patch that might have fixed it there?

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1469727 — Re: [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only open

FromJiri Kosina <jikos@kernel.org>
Date2016-08-24 23:20 +0200
SubjectRe: [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only open
Message-ID<s9NKx-7XX-11@gated-at.bofh.it>
In reply to#1469691
On Wed, 24 Aug 2016, Greg Kroah-Hartman wrote:

> > I have a problem with this patch. It only fixes one of the regressions
> > caused by the original change to the floppy driver. It does not address the
> > user land breakage of removing the NODELAY flag checks.
> 
> Does the same problem also happen in Linus's tree?  If not, any hints on
> the patch that might have fixed it there?

That's still an unresolved issue, and it's on my list to things to look 
into.

This particular patch though fixes a different issue, and should be 
applied to -stable.

Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1470129 — Re: [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only open

FromMark Hounschell <markh@compro.net>
Date2016-08-25 15:10 +0200
SubjectRe: [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only open
Message-ID<sa2zT-1n5-19@gated-at.bofh.it>
In reply to#1469727
On 08/24/2016 05:11 PM, Jiri Kosina wrote:
> On Wed, 24 Aug 2016, Greg Kroah-Hartman wrote:
>
>>> I have a problem with this patch. It only fixes one of the regressions
>>> caused by the original change to the floppy driver. It does not address the
>>> user land breakage of removing the NODELAY flag checks.
>>
>> Does the same problem also happen in Linus's tree?  If not, any hints on
>> the patch that might have fixed it there?
>

Yes, it does. IMHO the entire patch (between 4.4 and 4.5 that broke user 
land multiple ways) should be reverted and a fix for what ever obscure 
BUG was supposed to be fixed should be "retried".

> That's still an unresolved issue, and it's on my list to things to look
> into.
>
> This particular patch though fixes a different issue, and should be
> applied to -stable.
>

This patch fixes a bug that was introduced to fix "some other obscure 
BUG" that has nothing to do whatever with the "physical floppy" device. 
And it broke user land in at least 2 ways. All this patch does is revert 
part of the original patch so that it is only broke in 1 way to user 
land. It should revert the whole thing IMHO.

Regards
Mark

> Thanks,
>

[toc] | [prev] | [next] | [standalone]

#1470198 — Re: [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only open

FromMark Hounschell <markh@compro.net>
Date2016-08-25 17:00 +0200
SubjectRe: [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only open
Message-ID<sa4il-2hA-9@gated-at.bofh.it>
In reply to#1470129
On 08/25/2016 10:41 AM, Jens Axboe wrote:
> On 08/25/2016 07:08 AM, Mark Hounschell wrote:
>> On 08/24/2016 05:11 PM, Jiri Kosina wrote:
>>> On Wed, 24 Aug 2016, Greg Kroah-Hartman wrote:
>>>
>>>>> I have a problem with this patch. It only fixes one of the regressions
>>>>> caused by the original change to the floppy driver. It does not
>>>>> address the
>>>>> user land breakage of removing the NODELAY flag checks.
>>>>
>>>> Does the same problem also happen in Linus's tree?  If not, any
>>>> hints on
>>>> the patch that might have fixed it there?
>>>
>>
>> Yes, it does. IMHO the entire patch (between 4.4 and 4.5 that broke user
>> land multiple ways) should be reverted and a fix for what ever obscure
>> BUG was supposed to be fixed should be "retried".
>>
>>> That's still an unresolved issue, and it's on my list to things to look
>>> into.
>>>
>>> This particular patch though fixes a different issue, and should be
>>> applied to -stable.
>>>
>>
>> This patch fixes a bug that was introduced to fix "some other obscure
>> BUG" that has nothing to do whatever with the "physical floppy" device.
>> And it broke user land in at least 2 ways. All this patch does is revert
>> part of the original patch so that it is only broke in 1 way to user
>> land. It should revert the whole thing IMHO.
>
> Which patch is this? If that is truly the case, it should be reverted
> asap.
>
commit 09954bad448791ef01202351d437abdd9497a804 seems to be the one.

Mark

[toc] | [prev] | [next] | [standalone]

#1470213 — Re: [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only open

FromJiri Kosina <jikos@kernel.org>
Date2016-08-25 17:20 +0200
SubjectRe: [PATCH 4.7 146/186] floppy: fix open(O_ACCMODE) for ioctl-only open
Message-ID<sa4BI-2Is-27@gated-at.bofh.it>
In reply to#1470129
On Thu, 25 Aug 2016, Jens Axboe wrote:

> Which patch is this? If that is truly the case, it should be reverted 
> asap.

I've been looking into this for the past few days, and unfortunately I am 
not able to come up with a simple fix, so revert seems like the way to go.

Unfortunately, by reverting the patch, we'll get back into the original 
situation, where the bug found by syzkaller would be present.

If going down the revert path, we'd have to bring the driver to the state 
it was before, i.e. reverting it and the followup fix, i.e.

	ff06db1efb2ad6db06eb5b99b88a0c15a9cc9b0e
	09954bad448791ef01202351d437abdd9497a804

And then go back to the drawing board to fix the corruption reported by 
syzkaller.

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web