[patch NOT added to 3.12 stable tree] Don't feed anything but regular iovec's to blk_rq_map_user_iov

[patch NOT added to 3.12 stable tree] Don't feed anything but regular iovec's to blk_rq_map_user_iov

[Jiri Slaby]

From: Linus Torvalds <torvalds@linux-foundation.org>

This patch does NOT apply to the 3.12 stable tree. If you still want
it applied, please provide a backport.

===============

commit a0ac402cfcdc904f9772e1762b3fda112dcc56a0 upstream.

In theory we could map other things, but there's a reason that function
is called "user_iov".  Using anything else (like splice can do) just
confuses it.

Reported-and-tested-by: Johannes Thumshirn <jthumshirn@suse.de>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 block/blk-map.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/blk-map.c b/block/blk-map.c
index b8657fa8dc9a..27fd8d92892d 100644
--- a/block/blk-map.c
+++ b/block/blk-map.c
@@ -118,6 +118,9 @@ int blk_rq_map_user_iov(struct request_queue *q, struct request *rq,
 	struct iov_iter i;
 	int ret;
 
+	if (!iter_is_iovec(iter))
+		goto fail;
+
 	if (map_data)
 		copy = true;
 	else if (iov_iter_alignment(iter) & align)
@@ -140,6 +143,7 @@ int blk_rq_map_user_iov(struct request_queue *q, struct request *rq,
 
 unmap_rq:
 	__blk_rq_unmap_user(bio);
+fail:
 	rq->bio = NULL;
 	return -EINVAL;
 }
-- 
2.11.0

Re: [patch NOT added to 3.12 stable tree] Don't feed anything but regular iovec's to blk_rq_map_user_iov

[Greg KH]

On Mon, Dec 12, 2016 at 04:07:27PM +0100, Jiri Slaby wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> 
> This patch does NOT apply to the 3.12 stable tree. If you still want
> it applied, please provide a backport.
> 
> ===============
> 
> commit a0ac402cfcdc904f9772e1762b3fda112dcc56a0 upstream.
> 
> In theory we could map other things, but there's a reason that function
> is called "user_iov".  Using anything else (like splice can do) just
> confuses it.
> 
> Reported-and-tested-by: Johannes Thumshirn <jthumshirn@suse.de>
> Cc: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
>  block/blk-map.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/block/blk-map.c b/block/blk-map.c
> index b8657fa8dc9a..27fd8d92892d 100644
> --- a/block/blk-map.c
> +++ b/block/blk-map.c
> @@ -118,6 +118,9 @@ int blk_rq_map_user_iov(struct request_queue *q, struct request *rq,
>  	struct iov_iter i;
>  	int ret;
>  
> +	if (!iter_is_iovec(iter))
> +		goto fail;
> +
>  	if (map_data)
>  		copy = true;
>  	else if (iov_iter_alignment(iter) & align)
> @@ -140,6 +143,7 @@ int blk_rq_map_user_iov(struct request_queue *q, struct request *rq,
>  
>  unmap_rq:
>  	__blk_rq_unmap_user(bio);
> +fail:
>  	rq->bio = NULL;
>  	return -EINVAL;
>  }

I had to re-write this, feel free to steal the version I wrote for
4.4.y, it's commit d41fb2fbb28d3a1085960edada6c046becd1fafd in the
linux-stable tree.

thanks,

greg k-h

Re: [patch NOT added to 3.12 stable tree] Don't feed anything but regular iovec's to blk_rq_map_user_iov

[Jiri Slaby]

On 12/12/2016, 11:49 PM, Greg KH wrote:
> On Mon, Dec 12, 2016 at 04:07:27PM +0100, Jiri Slaby wrote:
>> From: Linus Torvalds <torvalds@linux-foundation.org>
>>
>> This patch does NOT apply to the 3.12 stable tree. If you still want
>> it applied, please provide a backport.
>>
>> ===============
>>
>> commit a0ac402cfcdc904f9772e1762b3fda112dcc56a0 upstream.
>>
>> In theory we could map other things, but there's a reason that function
>> is called "user_iov".  Using anything else (like splice can do) just
>> confuses it.
>>
>> Reported-and-tested-by: Johannes Thumshirn <jthumshirn@suse.de>
>> Cc: Al Viro <viro@ZenIV.linux.org.uk>
>> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>> ---
>>  block/blk-map.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/block/blk-map.c b/block/blk-map.c
>> index b8657fa8dc9a..27fd8d92892d 100644
>> --- a/block/blk-map.c
>> +++ b/block/blk-map.c
>> @@ -118,6 +118,9 @@ int blk_rq_map_user_iov(struct request_queue *q, struct request *rq,
>>  	struct iov_iter i;
>>  	int ret;
>>  
>> +	if (!iter_is_iovec(iter))
>> +		goto fail;
>> +
>>  	if (map_data)
>>  		copy = true;
>>  	else if (iov_iter_alignment(iter) & align)
>> @@ -140,6 +143,7 @@ int blk_rq_map_user_iov(struct request_queue *q, struct request *rq,
>>  
>>  unmap_rq:
>>  	__blk_rq_unmap_user(bio);
>> +fail:
>>  	rq->bio = NULL;
>>  	return -EINVAL;
>>  }
> 
> I had to re-write this, feel free to steal the version I wrote for
> 4.4.y, it's commit d41fb2fbb28d3a1085960edada6c046becd1fafd in the
> linux-stable tree.

I tried, but unfortunately, 3.12 has no iter_is_iovec :(.

thanks,
-- 
js
suse labs

Re: [patch NOT added to 3.12 stable tree] Don't feed anything but regular iovec's to blk_rq_map_user_iov

[Greg KH]

On Tue, Dec 13, 2016 at 03:40:53PM +0100, Jiri Slaby wrote:
> On 12/12/2016, 11:49 PM, Greg KH wrote:
> > On Mon, Dec 12, 2016 at 04:07:27PM +0100, Jiri Slaby wrote:
> >> From: Linus Torvalds <torvalds@linux-foundation.org>
> >>
> >> This patch does NOT apply to the 3.12 stable tree. If you still want
> >> it applied, please provide a backport.
> >>
> >> ===============
> >>
> >> commit a0ac402cfcdc904f9772e1762b3fda112dcc56a0 upstream.
> >>
> >> In theory we could map other things, but there's a reason that function
> >> is called "user_iov".  Using anything else (like splice can do) just
> >> confuses it.
> >>
> >> Reported-and-tested-by: Johannes Thumshirn <jthumshirn@suse.de>
> >> Cc: Al Viro <viro@ZenIV.linux.org.uk>
> >> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> >> ---
> >>  block/blk-map.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/block/blk-map.c b/block/blk-map.c
> >> index b8657fa8dc9a..27fd8d92892d 100644
> >> --- a/block/blk-map.c
> >> +++ b/block/blk-map.c
> >> @@ -118,6 +118,9 @@ int blk_rq_map_user_iov(struct request_queue *q, struct request *rq,
> >>  	struct iov_iter i;
> >>  	int ret;
> >>  
> >> +	if (!iter_is_iovec(iter))
> >> +		goto fail;
> >> +
> >>  	if (map_data)
> >>  		copy = true;
> >>  	else if (iov_iter_alignment(iter) & align)
> >> @@ -140,6 +143,7 @@ int blk_rq_map_user_iov(struct request_queue *q, struct request *rq,
> >>  
> >>  unmap_rq:
> >>  	__blk_rq_unmap_user(bio);
> >> +fail:
> >>  	rq->bio = NULL;
> >>  	return -EINVAL;
> >>  }
> > 
> > I had to re-write this, feel free to steal the version I wrote for
> > 4.4.y, it's commit d41fb2fbb28d3a1085960edada6c046becd1fafd in the
> > linux-stable tree.
> 
> I tried, but unfortunately, 3.12 has no iter_is_iovec :(.

And you can't "open code" it for 3.12 by just checking the bitfields?  I
haven't looked at 3.12 in a long time and I know the iovec code has
changed a lot, sorry if this is a stupid question.

thanks,

greg k-h

Re: [patch NOT added to 3.12 stable tree] Don't feed anything but regular iovec's to blk_rq_map_user_iov

[Jiri Slaby]

On 12/13/2016, 04:24 PM, Greg KH wrote:
>>>> --- a/block/blk-map.c
>>>> +++ b/block/blk-map.c
>>>> @@ -118,6 +118,9 @@ int blk_rq_map_user_iov(struct request_queue *q, struct request *rq,
>>>>  	struct iov_iter i;
>>>>  	int ret;
>>>>  
>>>> +	if (!iter_is_iovec(iter))
>>>> +		goto fail;
>>>> +
...
>> I tried, but unfortunately, 3.12 has no iter_is_iovec :(.
> 
> And you can't "open code" it for 3.12 by just checking the bitfields?

Nope, blk_rq_map_user_iov above does not even have "const struct
iov_iter *iter" as a parameter yet. Instead, "struct sg_iovec *iov" is
there.

So maybe, the patch is not even needed. But whoever can tell me for
sure, please do so.

thanks,
-- 
js
suse labs

Re: [patch NOT added to 3.12 stable tree] Don't feed anything but regular iovec's to blk_rq_map_user_iov

[Linus Torvalds]

On Tue, Dec 13, 2016 at 7:32 AM, Jiri Slaby <jslaby@suse.cz> wrote:
>
> Nope, blk_rq_map_user_iov above does not even have "const struct
> iov_iter *iter" as a parameter yet. Instead, "struct sg_iovec *iov" is
> there.
>
> So maybe, the patch is not even needed. But whoever can tell me for
> sure, please do so.

The issue is that we don't wand to get there through splice() creating
an iov that has kernel addresses in it, but I don't think 3.12 can do
that anyway - it would need an explicit splice function in the file
operations.

So I think 3.12 is fine without this. Al?

            Linus

Re: [patch NOT added to 3.12 stable tree] Don't feed anything but regular iovec's to blk_rq_map_user_iov

[Al Viro]

On Tue, Dec 13, 2016 at 08:46:56AM -0800, Linus Torvalds wrote:
> On Tue, Dec 13, 2016 at 7:32 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> >
> > Nope, blk_rq_map_user_iov above does not even have "const struct
> > iov_iter *iter" as a parameter yet. Instead, "struct sg_iovec *iov" is
> > there.
> >
> > So maybe, the patch is not even needed. But whoever can tell me for
> > sure, please do so.
> 
> The issue is that we don't wand to get there through splice() creating
> an iov that has kernel addresses in it, but I don't think 3.12 can do
> that anyway - it would need an explicit splice function in the file
> operations.

Not really - for 3.12 NULL ->splice_write() means going for
default_file_splice_write(), which will call write_pipe_buf() on the kmapped
pipe buffers, which will call __kernel_write(), which will do
set_fs(get_ds()); and call ->write().

And sg_write() called under set_fs(KERNEL_DS) is vulnerable there as well -
if nothing else, sg_write() -> sg_new_write():
694)        if (__copy_from_user(hp, buf, SZ_SG_IO_HDR)) {
...
722)        if (!access_ok(VERIFY_READ, hp->cmdp, hp->cmd_len)) {
723)                sg_remove_request(sfp, srp);
724)                return -EFAULT; /* protects following copy_from_user()s + get_user()s */
725)        }
726)        if (__copy_from_user(cmnd, hp->cmdp, hp->cmd_len)) {
already gives you read from arbitrary kernel address, and when you get
to sg_start_req() you have
701)                iov = memdup_user(hp->dxferp, size);
702)                if (IS_ERR(iov))
703)                        return PTR_ERR(iov);
704) 
705)                len = iov_length(iov, iov_count);
706)                if (hp->dxfer_len < len) {
707)                        iov_count = iov_shorten(iov, iov_count, hp->dxfer_len);
708)                        len = hp->dxfer_len;
709)                }
710) 
711)                res = blk_rq_map_user_iov(q, rq, md, (struct sg_iovec *)iov,
712)                                          iov_count,
713)                                          len, GFP_ATOMIC);
and there's your kernel-backed iovec passed to blk_rq_map_user_iov().  All
access_ok() will pass, of course, and if you have misaligned iovec array
element you'll force it into __bio_copy_vec(), with copy_{to,from}_user()
on user-supplied kernel address under set_fs(KERNEL_DS).  IOW, reads
and writes at arbitrary kernel address...

> So I think 3.12 is fine without this. Al?

I really doubt it - there might be something subtle I'd missed, but AFAICS
it is vulnerable to the scenario above.

Re: [patch NOT added to 3.12 stable tree] Don't feed anything but regular iovec's to blk_rq_map_user_iov

[Linus Torvalds]

On Tue, Dec 13, 2016 at 9:05 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> I really doubt it - there might be something subtle I'd missed, but AFAICS
> it is vulnerable to the scenario above.

Hmm. So maybe just add

        if (segment_eq(get_fs(), KERNEL_DS))
                return -EINVAL;

to blk_rq_map_user_iov()?

            Linus

Re: [patch NOT added to 3.12 stable tree] Don't feed anything but regular iovec's to blk_rq_map_user_iov

[Jiri Slaby]

On 12/13/2016, 06:09 PM, Linus Torvalds wrote:
> On Tue, Dec 13, 2016 at 9:05 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>
>> I really doubt it - there might be something subtle I'd missed, but AFAICS
>> it is vulnerable to the scenario above.
> 
> Hmm. So maybe just add
> 
>         if (segment_eq(get_fs(), KERNEL_DS))
>                 return -EINVAL;
> 
> to blk_rq_map_user_iov()?

BTW, is this a fix (or related):
https://patchwork.kernel.org/patch/9467743/
?

This seems to land into SUSE repositories as a fix for the CVE if I am
looking correctly.

thanks,
-- 
js
suse labs

Re: [patch NOT added to 3.12 stable tree] Don't feed anything but regular iovec's to blk_rq_map_user_iov

[Linus Torvalds]

On Tue, Dec 13, 2016 at 9:12 AM, Jiri Slaby <jslaby@suse.cz> wrote:
>
> BTW, is this a fix (or related):
> https://patchwork.kernel.org/patch/9467743/

That's a "we're probably going to restrict splice to people who
explicitly sign off on it". But not in that particular format.

It should fix the issue too.

               Linus

Re: [patch NOT added to 3.12 stable tree] Don't feed anything but regular iovec's to blk_rq_map_user_iov

[Al Viro]

On Tue, Dec 13, 2016 at 09:09:25AM -0800, Linus Torvalds wrote:
> On Tue, Dec 13, 2016 at 9:05 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > I really doubt it - there might be something subtle I'd missed, but AFAICS
> > it is vulnerable to the scenario above.
> 
> Hmm. So maybe just add
> 
>         if (segment_eq(get_fs(), KERNEL_DS))
>                 return -EINVAL;
> 
> to blk_rq_map_user_iov()?

To sg_write(), actually.  And we need it both in old branches and in
the mainline - sg_new_write() problem applies in mainline as well, and
if nothing else, it allows to perform reads from arbitrary kernel
space address; I'm not sure how easy it is to see the value it has
fetched, but it certainly can do unpleasant things to memory-mapped
IO registers.

As the matter of fact, bsg_write() is no better - blk_fill_sgv4_hdr_rq()
is called before we get to blk_rq_map_user() where your test would make
it fail, and it does
        if (copy_from_user(rq->cmd, (void __user *)(unsigned long)hdr->request,
                           hdr->request_len))   
                return -EFAULT;
with *hdr, including hrd->request, has come from the data fed to bsg_write().

So I think we should make both sg_write() and bsg_write() to fail as early
as possible when called with KERNEL_DS, mainline and all -stable branches.