LinuxLists.cc - fs: out of bounds on stack in iov_iter

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

On Mon, Aug 17, 2015 at 12:18:12PM +0300, Andrey Ryabinin wrote:

> This bug is similar to recently found bug in 9p: http://thread.gmane.org/gmane.linux.kernel/1931799/focus=1936542

Ow. For those who'd missed that fun: the bug in question had turned out to
be caused by improper reuse of request ids, _not_ in the call chain of
the triggering syscall.

> if (!retval) {
> struct iov_iter data = *iter;
> retval = mapping->a_ops->direct_IO(iocb, &data, pos);
> }
>
> if (retval > 0) {
> *ppos = pos + retval;
> iov_iter_advance(iter, retval);
>
>
> So either filemap_write_and_wait_range()
Shouldn't - it's supposed to return 0 or -E...

> or mapping->a_ops->direct_IO() returned more
> than 'count'.

Was there DAX involved? ->direct_IO() in there is blkdev_direct_IO(),
which takes rather different paths in those cases...

> > Also too the file and line number
> > (lib/iov_iter.c:511) are completely useless because of inlining,
> > though that's not kasan's fault.

Might make sense to slap
if (WARN_ON(size > iov_iter_count(i)))
print size and *i
and see if it triggers...

2015-11-06 01:34:10

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

On Wed, Sep 30, 2015 at 05:30:17PM -0400, Sasha Levin wrote:

> > So I've traced this all the way back to dax_io(). I can trigger this with:
> >
> > diff --git a/fs/dax.c b/fs/dax.c
> > index 93bf2f9..2cdb8a5 100644
> > --- a/fs/dax.c
> > +++ b/fs/dax.c
> > @@ -178,6 +178,7 @@ static ssize_t dax_io(struct inode *inode, struct iov_iter *iter,
> > if (need_wmb)
> > wmb_pmem();
> >
> > + WARN_ON((pos == start) && (pos - start > iov_iter_count(iter)));
> > return (pos == start) ? retval : pos - start;
> > }
> >
> > So it seems that iter gets moved twice here: once in dax_io(), and once again
> > back at generic_file_read_iter().
> >
> > I don't see how it ever worked. Am I missing something?

This:
struct iov_iter data = *iter;
retval = mapping->a_ops->direct_IO(iocb, &data, pos);
}

if (retval > 0) {
*ppos = pos + retval;
iov_iter_advance(iter, retval);

The iterator advanced in ->direct_IO() is a _copy_, not the original.
The contents of *iter as seen by generic_file_read_iter() is not
modifiable by ->direct_IO(), simply because its address is nowhere to
be found. And checking iov_iter_count(iter) at the end of dax_io() is
pointless - from the POV of generic_file_read_iter() it's data.count,
and while it used to be equal to iter->count, it's already been modified.
By the time we call iov_iter_advance() in generic_file_read_iter() that
value will be already discarded, along with rest of struct iov_iter data.

Wait a minute - you are triggering _what_???
> > + WARN_ON((pos == start) && (pos - start > iov_iter_count(iter)));
With '&&'? iov_iter_count() is size_t, while pos and start are loff_t,
so you are seeing equal values in pos and start (as integers) *and*
(loff_t)0 > (size_t)something. loff_t is a signed type, size_t - unsigned.
6.3.1.8[1] says that
* if rank of size_t is greater or equal to rank of loff_t, the
latter gets converted to size_t. And conversion of zero should be zero,
i.e. (size_t) 0 > (size_t)something, which is impossible (we compare them
as non-negative integers).
* if loff_t can represent all values of size_t, size_t value gets
converted to loff_t. Result of conversion should have the same (in particular,
non-negative) value. Again, comparison can't be true.
* otherwise both values are converted to unsigned counterpart of
loff_t. Again, zero converts to 0 and in any unsigned type 0 > x is
impossible.

I don't see any way for that condition to evaluate true.

Assuming that it's a misquoted ||... I don't see any way for pos to
get greater than start + original iov_iter_count(). However, I *do*
see a way for bad things to happen in a different way. Look:
// first pass through the loop, pos == start (and so's max)
retval = dax_get_addr(bh, &addr, blkbits);
// got a positive value
if (retval < 0)
break;
// nope, keep going
if (buffer_unwritten(bh) || buffer_new(bh)) {
dax_new_buf(addr, retval, first, pos,
end);
need_wmb = true;
}
addr += first;
size = retval - first;
// OK...
}
max = min(pos + size, end);
// OK...
}

if (iov_iter_rw(iter) == WRITE) {
len = copy_from_iter_pmem(addr, max - pos, iter);
need_wmb = true;
} else if (!hole)
len = copy_to_iter((void __force *)addr, max - pos,
iter);
else
len = iov_iter_zero(max - pos, iter);
// too bad - we'd hit an unmapped memory area. len is 0...
// and retval is fucking positive.
if (!len)
break;

return (pos == start) ? retval : pos - start;
// will return a bloody big positive value

Could you try to reproduce it with this:

dax_io(): don't let non-error value escape via retval instead of EFAULT

Signed-off-by: Al Viro <[email protected]>
---
diff --git a/fs/dax.c b/fs/dax.c
index a86d3cc..7b653e9 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -169,8 +169,10 @@ static ssize_t dax_io(struct inode *inode, struct iov_iter *iter,
else
len = iov_iter_zero(max - pos, iter);

- if (!len)
+ if (!len) {
+ retval = -EFAULT;
break;
+ }

pos += len;
addr += len;

2015-11-06 02:19:11

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

On Fri, Nov 06, 2015 at 01:34:02AM +0000, Al Viro wrote:

> Could you try to reproduce it with this:
>
> dax_io(): don't let non-error value escape via retval instead of EFAULT
>
> Signed-off-by: Al Viro <[email protected]>
> ---
> diff --git a/fs/dax.c b/fs/dax.c
> index a86d3cc..7b653e9 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -169,8 +169,10 @@ static ssize_t dax_io(struct inode *inode, struct iov_iter *iter,
> else
> len = iov_iter_zero(max - pos, iter);
>
> - if (!len)
> + if (!len) {
> + retval = -EFAULT;
> break;
> + }
>
> pos += len;
> addr += len;
>

PS: "block, dax: fix lifetime of in-kernel dax mappings with dax_map_atomic()"
Dan Williams had posted a while ago does change the things a bit, but
AFAICS only in turning "return a bogus positive value" into "return an
uninitialized value"; if applying that one after it, s/retval/rc/ in
the above. And whether it fixes the bug Sasha had been able to trigger,
the bug is real and needs fixing - it's been there since 4.0 when fs/dax.c
went into the tree.

How are we going to handle that one? I can put it into mainline pull
request via vfs.git, with Cc: stable, but if e.g. Jens prefers to take it
via the block tree, I'll be glad to leave it for him to deal with.

2015-11-06 03:38:34

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

On Thu, Nov 5, 2015 at 6:19 PM, Al Viro <[email protected]> wrote:
>
> How are we going to handle that one? I can put it into mainline pull
> request via vfs.git, with Cc: stable, but if e.g. Jens prefers to take it
> via the block tree, I'll be glad to leave it for him to deal with.

Put it in the vfs tree (I'm hoping for a pull request soon..)

I pulled the block trees from Jens yesterday, so there is presumably
nothing pending there right now.

Linus

2015-11-06 16:06:58

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

On 11/05/2015 08:38 PM, Linus Torvalds wrote:
> On Thu, Nov 5, 2015 at 6:19 PM, Al Viro <[email protected]> wrote:
>>
>> How are we going to handle that one? I can put it into mainline pull
>> request via vfs.git, with Cc: stable, but if e.g. Jens prefers to take it
>> via the block tree, I'll be glad to leave it for him to deal with.
>
> Put it in the vfs tree (I'm hoping for a pull request soon..)
>
> I pulled the block trees from Jens yesterday, so there is presumably
> nothing pending there right now.

Either way is obviously fine with me. I have 4 patches pending, but
unless more urgent things show up, I was going to continue collecting
fixes and submit that post -rc1.

--
Jens Axboe

2015-11-11 02:21:50

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

Al, ping?

On Thu, Nov 5, 2015 at 7:38 PM, Linus Torvalds
<[email protected]> wrote:
> On Thu, Nov 5, 2015 at 6:19 PM, Al Viro <[email protected]> wrote:
>>
>> How are we going to handle that one? I can put it into mainline pull
>> request via vfs.git, with Cc: stable, but if e.g. Jens prefers to take it
>> via the block tree, I'll be glad to leave it for him to deal with.
>
> Put it in the vfs tree (I'm hoping for a pull request soon..)
>
> I pulled the block trees from Jens yesterday, so there is presumably
> nothing pending there right now.

Apparently my "hoping for a pull request soon" was ridiculously optimistic.

Al, looking at the most recent linux-next, most of the vfs commits
there seem to be committed in the last day or two. I'm getting the
feeling that that is all 4.5 material by now.

Should I just take the iov patch as-is, since apparently no vfs pull
request is happening this merge cycle? And no, I'm not taking
"developed during the second week of the merge window, and sent in the
last few days of it". I'm done with that.

Linus

2015-11-11 02:25:59

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

On Tue, Nov 10 2015, Linus Torvalds wrote:
> Al, ping?
>
> On Thu, Nov 5, 2015 at 7:38 PM, Linus Torvalds
> <[email protected]> wrote:
> > On Thu, Nov 5, 2015 at 6:19 PM, Al Viro <[email protected]> wrote:
> >>
> >> How are we going to handle that one? I can put it into mainline pull
> >> request via vfs.git, with Cc: stable, but if e.g. Jens prefers to take it
> >> via the block tree, I'll be glad to leave it for him to deal with.
> >
> > Put it in the vfs tree (I'm hoping for a pull request soon..)
> >
> > I pulled the block trees from Jens yesterday, so there is presumably
> > nothing pending there right now.
>
> Apparently my "hoping for a pull request soon" was ridiculously optimistic.
>
> Al, looking at the most recent linux-next, most of the vfs commits
> there seem to be committed in the last day or two. I'm getting the
> feeling that that is all 4.5 material by now.
>
> Should I just take the iov patch as-is, since apparently no vfs pull
> request is happening this merge cycle? And no, I'm not taking
> "developed during the second week of the merge window, and sent in the
> last few days of it". I'm done with that.

I've got 8 other patches pending for a post core merge, just waiting for
the last core pull request to go in. I haven't seen this iov iter fix,
though.

git://git.kernel.dk/linux-block.git for-linus

----------------------------------------------------------------
Jan Kara (1):
brd: Refuse improperly aligned discard requests

Jens Axboe (2):
MAINTAINERS: add reference to new linux-block list
blk-mq: mark __blk_mq_complete_request() static

Randy Dunlap (1):
block: fix blk-core.c kernel-doc warning

Sathyavathi M (1):
NVMe: Increase the max transfer size when mdts is 0

Stephan G?nther (2):
NVMe: use split lo_hi_{read,write}q
NVMe: add support for Apple NVMe controller

Vivek Goyal (1):
fs/block_dev.c: Remove WARN_ON() when inode writeback fails

MAINTAINERS | 1 +
block/blk-core.c | 3 +++
block/blk-mq.c | 2 +-
block/blk-mq.h | 1 -
drivers/block/brd.c | 3 +++
drivers/nvme/host/pci.c | 15 +++++++++------
fs/block_dev.c | 15 ++++++++++++---
7 files changed, 29 insertions(+), 11 deletions(-)

--
Jens Axboe

2015-11-11 02:31:51

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

On Tue, Nov 10, 2015 at 6:25 PM, Jens Axboe <[email protected]> wrote:
> On Tue, Nov 10 2015, Linus Torvalds wrote:
>> Al, ping?
>>
>> On Thu, Nov 5, 2015 at 7:38 PM, Linus Torvalds
>> <[email protected]> wrote:
>> > On Thu, Nov 5, 2015 at 6:19 PM, Al Viro <[email protected]> wrote:
>> >>
>> >> How are we going to handle that one? I can put it into mainline pull
>> >> request via vfs.git, with Cc: stable, but if e.g. Jens prefers to take it
>> >> via the block tree, I'll be glad to leave it for him to deal with.
>> >
>> > Put it in the vfs tree (I'm hoping for a pull request soon..)
>> >
>> > I pulled the block trees from Jens yesterday, so there is presumably
>> > nothing pending there right now.
>>
>> Apparently my "hoping for a pull request soon" was ridiculously optimistic.
>>
>> Al, looking at the most recent linux-next, most of the vfs commits
>> there seem to be committed in the last day or two. I'm getting the
>> feeling that that is all 4.5 material by now.
>>
>> Should I just take the iov patch as-is, since apparently no vfs pull
>> request is happening this merge cycle? And no, I'm not taking
>> "developed during the second week of the merge window, and sent in the
>> last few days of it". I'm done with that.
>
> I've got 8 other patches pending for a post core merge, just waiting for
> the last core pull request to go in. I haven't seen this iov iter fix,
> though.

It was in this thread, looked like this (without the whitespace damage):

dax_io(): don't let non-error value escape via retval instead of EFAULT

Signed-off-by: Al Viro <[email protected]>
---
diff --git a/fs/dax.c b/fs/dax.c
index a86d3cc..7b653e9 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -169,8 +169,10 @@ static ssize_t dax_io(struct inode *inode,
struct iov_iter *iter,
else
len = iov_iter_zero(max - pos, iter);

- if (!len)
+ if (!len) {
+ retval = -EFAULT;
break;
+ }

pos += len;
addr += len;

although I don't think I saw a confirmation that that was what Sasha
actually hit (but Sasha had narrowed it down to DAX, so it looks
possible/likely)

Linus

2015-11-11 02:40:27

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

On 11/10/2015 07:31 PM, Linus Torvalds wrote:
> On Tue, Nov 10, 2015 at 6:25 PM, Jens Axboe <[email protected]> wrote:
>> On Tue, Nov 10 2015, Linus Torvalds wrote:
>>> Al, ping?
>>>
>>> On Thu, Nov 5, 2015 at 7:38 PM, Linus Torvalds
>>> <[email protected]> wrote:
>>>> On Thu, Nov 5, 2015 at 6:19 PM, Al Viro <[email protected]> wrote:
>>>>>
>>>>> How are we going to handle that one? I can put it into mainline pull
>>>>> request via vfs.git, with Cc: stable, but if e.g. Jens prefers to take it
>>>>> via the block tree, I'll be glad to leave it for him to deal with.
>>>>
>>>> Put it in the vfs tree (I'm hoping for a pull request soon..)
>>>>
>>>> I pulled the block trees from Jens yesterday, so there is presumably
>>>> nothing pending there right now.
>>>
>>> Apparently my "hoping for a pull request soon" was ridiculously optimistic.
>>>
>>> Al, looking at the most recent linux-next, most of the vfs commits
>>> there seem to be committed in the last day or two. I'm getting the
>>> feeling that that is all 4.5 material by now.
>>>
>>> Should I just take the iov patch as-is, since apparently no vfs pull
>>> request is happening this merge cycle? And no, I'm not taking
>>> "developed during the second week of the merge window, and sent in the
>>> last few days of it". I'm done with that.
>>
>> I've got 8 other patches pending for a post core merge, just waiting for
>> the last core pull request to go in. I haven't seen this iov iter fix,
>> though.
>
> It was in this thread, looked like this (without the whitespace damage):
>
> dax_io(): don't let non-error value escape via retval instead of EFAULT
>
> Signed-off-by: Al Viro <[email protected]>
> ---
> diff --git a/fs/dax.c b/fs/dax.c
> index a86d3cc..7b653e9 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -169,8 +169,10 @@ static ssize_t dax_io(struct inode *inode,
> struct iov_iter *iter,
> else
> len = iov_iter_zero(max - pos, iter);
>
> - if (!len)
> + if (!len) {
> + retval = -EFAULT;
> break;
> + }
>
> pos += len;
> addr += len;
>
>
> although I don't think I saw a confirmation that that was what Sasha
> actually hit (but Sasha had narrowed it down to DAX, so it looks
> possible/likely)

I found it right after sending that email. Patch looks pretty straight
forward, at least from the case of max - pos != 0 and len == 0 on
return. Might be cleaner to add a

if (retval < 0)
break;

check, that should be the case where max == pos anyway. But we'd
potentially return -Exx into -EFAULT for that case with the patch.

Hmm?

--
Jens Axboe

2015-11-11 02:41:51

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

On 11/10/2015 07:40 PM, Jens Axboe wrote:
> On 11/10/2015 07:31 PM, Linus Torvalds wrote:
>> On Tue, Nov 10, 2015 at 6:25 PM, Jens Axboe <[email protected]> wrote:
>>> On Tue, Nov 10 2015, Linus Torvalds wrote:
>>>> Al, ping?
>>>>
>>>> On Thu, Nov 5, 2015 at 7:38 PM, Linus Torvalds
>>>> <[email protected]> wrote:
>>>>> On Thu, Nov 5, 2015 at 6:19 PM, Al Viro <[email protected]>
>>>>> wrote:
>>>>>>
>>>>>> How are we going to handle that one? I can put it into mainline pull
>>>>>> request via vfs.git, with Cc: stable, but if e.g. Jens prefers to
>>>>>> take it
>>>>>> via the block tree, I'll be glad to leave it for him to deal with.
>>>>>
>>>>> Put it in the vfs tree (I'm hoping for a pull request soon..)
>>>>>
>>>>> I pulled the block trees from Jens yesterday, so there is presumably
>>>>> nothing pending there right now.
>>>>
>>>> Apparently my "hoping for a pull request soon" was ridiculously
>>>> optimistic.
>>>>
>>>> Al, looking at the most recent linux-next, most of the vfs commits
>>>> there seem to be committed in the last day or two. I'm getting the
>>>> feeling that that is all 4.5 material by now.
>>>>
>>>> Should I just take the iov patch as-is, since apparently no vfs pull
>>>> request is happening this merge cycle? And no, I'm not taking
>>>> "developed during the second week of the merge window, and sent in the
>>>> last few days of it". I'm done with that.
>>>
>>> I've got 8 other patches pending for a post core merge, just waiting for
>>> the last core pull request to go in. I haven't seen this iov iter fix,
>>> though.
>>
>> It was in this thread, looked like this (without the whitespace damage):
>>
>> dax_io(): don't let non-error value escape via retval instead of
>> EFAULT
>>
>> Signed-off-by: Al Viro <[email protected]>
>> ---
>> diff --git a/fs/dax.c b/fs/dax.c
>> index a86d3cc..7b653e9 100644
>> --- a/fs/dax.c
>> +++ b/fs/dax.c
>> @@ -169,8 +169,10 @@ static ssize_t dax_io(struct inode *inode,
>> struct iov_iter *iter,
>> else
>> len = iov_iter_zero(max - pos, iter);
>>
>> - if (!len)
>> + if (!len) {
>> + retval = -EFAULT;
>> break;
>> + }
>>
>> pos += len;
>> addr += len;
>>
>>
>> although I don't think I saw a confirmation that that was what Sasha
>> actually hit (but Sasha had narrowed it down to DAX, so it looks
>> possible/likely)
>
> I found it right after sending that email. Patch looks pretty straight
> forward, at least from the case of max - pos != 0 and len == 0 on
> return. Might be cleaner to add a
>
> if (retval < 0)
> break;
>
> check, that should be the case where max == pos anyway. But we'd
> potentially return -Exx into -EFAULT for that case with the patch.
>
> Hmm?

So we already do that, in the 'if' above. I think the patch looks fine.

--
Jens Axboe

2015-11-11 02:44:22

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

On 11/10/2015 07:41 PM, Jens Axboe wrote:
> On 11/10/2015 07:40 PM, Jens Axboe wrote:
>> On 11/10/2015 07:31 PM, Linus Torvalds wrote:
>>> On Tue, Nov 10, 2015 at 6:25 PM, Jens Axboe <[email protected]> wrote:
>>>> On Tue, Nov 10 2015, Linus Torvalds wrote:
>>>>> Al, ping?
>>>>>
>>>>> On Thu, Nov 5, 2015 at 7:38 PM, Linus Torvalds
>>>>> <[email protected]> wrote:
>>>>>> On Thu, Nov 5, 2015 at 6:19 PM, Al Viro <[email protected]>
>>>>>> wrote:
>>>>>>>
>>>>>>> How are we going to handle that one? I can put it into mainline
>>>>>>> pull
>>>>>>> request via vfs.git, with Cc: stable, but if e.g. Jens prefers to
>>>>>>> take it
>>>>>>> via the block tree, I'll be glad to leave it for him to deal with.
>>>>>>
>>>>>> Put it in the vfs tree (I'm hoping for a pull request soon..)
>>>>>>
>>>>>> I pulled the block trees from Jens yesterday, so there is presumably
>>>>>> nothing pending there right now.
>>>>>
>>>>> Apparently my "hoping for a pull request soon" was ridiculously
>>>>> optimistic.
>>>>>
>>>>> Al, looking at the most recent linux-next, most of the vfs commits
>>>>> there seem to be committed in the last day or two. I'm getting the
>>>>> feeling that that is all 4.5 material by now.
>>>>>
>>>>> Should I just take the iov patch as-is, since apparently no vfs pull
>>>>> request is happening this merge cycle? And no, I'm not taking
>>>>> "developed during the second week of the merge window, and sent in the
>>>>> last few days of it". I'm done with that.
>>>>
>>>> I've got 8 other patches pending for a post core merge, just waiting
>>>> for
>>>> the last core pull request to go in. I haven't seen this iov iter fix,
>>>> though.
>>>
>>> It was in this thread, looked like this (without the whitespace damage):
>>>
>>> dax_io(): don't let non-error value escape via retval instead of
>>> EFAULT
>>>
>>> Signed-off-by: Al Viro <[email protected]>
>>> ---
>>> diff --git a/fs/dax.c b/fs/dax.c
>>> index a86d3cc..7b653e9 100644
>>> --- a/fs/dax.c
>>> +++ b/fs/dax.c
>>> @@ -169,8 +169,10 @@ static ssize_t dax_io(struct inode *inode,
>>> struct iov_iter *iter,
>>> else
>>> len = iov_iter_zero(max - pos, iter);
>>>
>>> - if (!len)
>>> + if (!len) {
>>> + retval = -EFAULT;
>>> break;
>>> + }
>>>
>>> pos += len;
>>> addr += len;
>>>
>>>
>>> although I don't think I saw a confirmation that that was what Sasha
>>> actually hit (but Sasha had narrowed it down to DAX, so it looks
>>> possible/likely)
>>
>> I found it right after sending that email. Patch looks pretty straight
>> forward, at least from the case of max - pos != 0 and len == 0 on
>> return. Might be cleaner to add a
>>
>> if (retval < 0)
>> break;
>>
>> check, that should be the case where max == pos anyway. But we'd
>> potentially return -Exx into -EFAULT for that case with the patch.
>>
>> Hmm?
>
> So we already do that, in the 'if' above. I think the patch looks fine.

Queued up. Unless Al objects, it'll be part of the 'for-linus' pull
later this week.

--
Jens Axboe

2015-11-11 02:56:56

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

On Tue, Nov 10, 2015 at 06:21:47PM -0800, Linus Torvalds wrote:

> Al, looking at the most recent linux-next, most of the vfs commits
> there seem to be committed in the last day or two. I'm getting the
> feeling that that is all 4.5 material by now.
>
> Should I just take the iov patch as-is, since apparently no vfs pull
> request is happening this merge cycle? And no, I'm not taking
> "developed during the second week of the merge window, and sent in the
> last few days of it". I'm done with that.

s/developed/rebased/, actually, but... point taken. Mea culpa, and what
to do with those patches is for you to decide; some of those are simply
-stable fodder and probably ought to go one-by-one at any point you would
consider convenient, some are of the "remove stale comment" variety (obviously
can sit around until the next cycle, or go in one-by-one at any point - the
things like
-
- /* WARNING: probably going away soon, do not use! */
in inode_operations; the comment used to be about the method removed last
cycle and should've been gone with it; etc.)

There's a large pile not in those two classes - xattr+richacl stuff. I'm more
confident about the first part, but strictly speaking neither qualifies as
fixes.

FWIW, the stuff that had been _developed_ during the merge window is not there
- a patch series around the descriptor bitmaps. Doesn't change the situation;
I'd fucked up this cycle ;-/

2015-11-11 03:06:15

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

On Tue, Nov 10, 2015 at 07:44:14PM -0700, Jens Axboe wrote:

> Queued up. Unless Al objects, it'll be part of the 'for-linus' pull
> later this week.

Reported-by: Sasha Levin <[email protected]>
Cc: [email protected] # 4.0+

probably ought to be there...

2015-11-11 03:08:00

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

On 11/10/2015 08:06 PM, Al Viro wrote:
> On Tue, Nov 10, 2015 at 07:44:14PM -0700, Jens Axboe wrote:
>
>> Queued up. Unless Al objects, it'll be part of the 'for-linus' pull
>> later this week.
>
> Reported-by: Sasha Levin <[email protected]>
> Cc: [email protected] # 4.0+
>
> probably ought to be there...

Agree, done.

--
Jens Axboe

2015-11-11 03:21:06

by Sasha Levin

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

On 11/10/2015 09:31 PM, Linus Torvalds wrote:
> although I don't think I saw a confirmation that that was what Sasha
> actually hit (but Sasha had narrowed it down to DAX, so it looks
> possible/likely)

Yup, that indeed fixed the problem I was seeing.

Thanks,
Sasha

2015-11-11 03:30:15

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

On Wed, Nov 11, 2015 at 02:56:47AM +0000, Al Viro wrote:
> s/developed/rebased/, actually, but... point taken. Mea culpa, and what
> to do with those patches is for you to decide; some of those are simply
> -stable fodder and probably ought to go one-by-one at any point you would
> consider convenient, some are of the "remove stale comment" variety (obviously
> can sit around until the next cycle, or go in one-by-one at any point - the
> things like
> -
> - /* WARNING: probably going away soon, do not use! */
> in inode_operations; the comment used to be about the method removed last
> cycle and should've been gone with it; etc.)

FWIW, here's what's in there:
dax_io fix
Jens has just taken it
fs: fix inode.c kernel-doc warning
fs: fix writeback.c kernel-doc warnings
trivial comment patches
overlayfs: move super block magic number to magic.h
got picked into overlayfs tree yesterday
debugfs: fix refcount imbalance in start_creating
old fix, -stable fodder (had been first posted in October, IIRC)
vfs: Check attribute names in posix acl xattr handers
vfs: Fix the posix_acl_xattr_list return value
ubifs: Remove unused security xattr handler
hfsplus: Remove unused xattr handler list operations
jffs2: Add missing capability check for listing trusted xattrs
xattr handlers: Pass handler to operations instead of flags
9p: xattr simplifications
squashfs: xattr simplifications
f2fs: xattr simplifications
xattr series; the first two are arguably fixes, and whatever happens in this
window, I'm taking the rest into -next for 4.5. Series makes sense and
cleans the things nicely, IMO.
FS-Cache: Increase reference of parent after registering, netfs success
FS-Cache: Don't override netfs's primary_index if registering failed
cachefiles: perform test on s_blocksize when opening cache file.
FS-Cache: Handle a write to the page immediately beyond the EOF marker
1, 2 and 4 are simply -stable fodder, 3 is an obvious optimization.
binfmt_elf: Don't clobber passed executable's file header
binfmt_elf: Correct `arch_check_elf's description
-stable fodder.
fs/pipe.c: preserve alloc_file() error code
fs/pipe.c: return error code rather than 0 in pipe_write()
-stable fodder.
vfs: remove unused wrapper block_page_mkwrite()
vfs: remove stale comment in inode_operations
dead code and stale comment removal. Can go at any point.
fs: 9p: cache.h: Add #define of include guard
trivial, can go at any point, or stay until the next cycle.
richacl series
probably misses the window - I'd really like to hear more detailed variant
of Christoph's objections in any case.

Again, my apologies to everyone involved - I'd fucked up, badly. The only
question is how much PITA it will end up causing. I can put those into
separate branches and/or mail directly; what ends up missing the window
will go into vfs.git#for-next as soon as -rc1 is out there (with the
possible exception of richacl stuff - I really want to hear from Christoph
and in more details than "it's all been said some iterations ago").

Linus, what would be your preference wrt that stuff? Besides the "don't
ever do that kind of shit again", that is - that much is obvious.

2015-11-11 04:36:50

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

On Tue, Nov 10, 2015 at 7:30 PM, Al Viro <[email protected]> wrote:
>
> Linus, what would be your preference wrt that stuff?

If you can just create a branch with the stuff that is obvious and
clearly worth it (ie stuff that would basically be stable material
anyway), I'll just merge it. Assuming it's all done in some
reasonable timeframe..

Linus

2015-11-11 07:43:40

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

On Tue, Nov 10, 2015 at 08:36:48PM -0800, Linus Torvalds wrote:
> On Tue, Nov 10, 2015 at 7:30 PM, Al Viro <[email protected]> wrote:
> >
> > Linus, what would be your preference wrt that stuff?
>
> If you can just create a branch with the stuff that is obvious and
> clearly worth it (ie stuff that would basically be stable material
> anyway), I'll just merge it. Assuming it's all done in some
> reasonable timeframe..

OK... Right now I have #for-linus-stable and #for-linus-2 on top
of it, the latter adding several comment fixes, etc., the most serious
change among which is the removal of never used block_page_mkwrite().

dax_io fix isn't there, neither is overlayfs magic.h patch - both are
already in other trees. I would like to get xattr series in as well,
but that's a separate pull request, if you'd accept them in this window in
the first place. richacl stuff isn't there as well, and I think that one
is clear "leave it for 4.5" fodder.

Anyway, for
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-linus-2
(both -stable fodder and trivial patches)

Shortlog:
Daniel Borkmann (1):
debugfs: fix refcount imbalance in start_creating

David Howells (1):
FS-Cache: Handle a write to the page immediately beyond the EOF marker

Eric Biggers (2):
fs/pipe.c: preserve alloc_file() error code
fs/pipe.c: return error code rather than 0 in pipe_write()

Kinglong Mee (2):
FS-Cache: Increase reference of parent after registering, netfs success
FS-Cache: Don't override netfs's primary_index if registering failed

Maciej W. Rozycki (2):
binfmt_elf: Don't clobber passed executable's file header
binfmt_elf: Correct `arch_check_elf's description

NeilBrown (1):
cachefiles: perform test on s_blocksize when opening cache file.

Randy Dunlap (2):
fs: fix inode.c kernel-doc warning
fs: fix writeback.c kernel-doc warnings

Ross Zwisler (2):
vfs: remove unused wrapper block_page_mkwrite()
vfs: remove stale comment in inode_operations

Tzvetelin Katchov (1):
fs: 9p: cache.h: Add #define of include guard

Diffstat:
fs/9p/cache.h | 1 +
fs/binfmt_elf.c | 12 ++++----
fs/buffer.c | 24 ++-------------
fs/cachefiles/namei.c | 2 ++
fs/cachefiles/rdwr.c | 73 +++++++++++++++++++++++----------------------
fs/debugfs/inode.c | 6 +++-
fs/ext4/inode.c | 4 +--
fs/fs-writeback.c | 3 +-
fs/fscache/netfs.c | 38 +++++++++++------------
fs/fscache/page.c | 2 +-
fs/inode.c | 1 +
fs/nilfs2/file.c | 2 +-
fs/pipe.c | 18 ++++++-----
fs/xfs/xfs_file.c | 2 +-
include/linux/buffer_head.h | 2 --
include/linux/fs.h | 2 --
16 files changed, 89 insertions(+), 103 deletions(-)

If you'd prefer to do that in two separate pulls - yell (or just pull
#for-linux-stable first, then this on top of it). I'd reordered
#for-next so that it continues #for-linus-2; tree of its tip being the
same as yesterday.

2015-11-11 08:16:45

by Stephen Rothwell

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

Hi Al,

On Wed, 11 Nov 2015 07:43:30 +0000 Al Viro <[email protected]> wrote:
>
> dax_io fix isn't there, neither is overlayfs magic.h patch - both are
> already in other trees. I would like to get xattr series in as well,
> but that's a separate pull request, if you'd accept them in this window in
> the first place. richacl stuff isn't there as well, and I think that one
> is clear "leave it for 4.5" fodder.

So could you please remove the 4.5 stuff from your for-next branch
until after the merge window closes.

Also, I noticed these new warnings today:

fs/orangefs/xattr.c:509:9: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
.get = pvfs2_xattr_get_trusted,
^
fs/orangefs/xattr.c:509:9: note: (near initialization for 'pvfs2_xattr_trusted_handler.get')
fs/orangefs/xattr.c:510:9: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
.set = pvfs2_xattr_set_trusted,
^
fs/orangefs/xattr.c:510:9: note: (near initialization for 'pvfs2_xattr_trusted_handler.set')
fs/orangefs/xattr.c:520:9: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
.get = pvfs2_xattr_get_default,
^
fs/orangefs/xattr.c:520:9: note: (near initialization for 'pvfs2_xattr_default_handler.get')
fs/orangefs/xattr.c:521:9: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
.set = pvfs2_xattr_set_default,
^
fs/orangefs/xattr.c:521:9: note: (near initialization for 'pvfs2_xattr_default_handler.set')

--
Cheers,
Stephen Rothwell [email protected]

2015-11-11 10:20:03

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

On Wed, Nov 11, 2015 at 07:16:36PM +1100, Stephen Rothwell wrote:
> Hi Al,
>
> On Wed, 11 Nov 2015 07:43:30 +0000 Al Viro <[email protected]> wrote:
> >
> > dax_io fix isn't there, neither is overlayfs magic.h patch - both are
> > already in other trees. I would like to get xattr series in as well,
> > but that's a separate pull request, if you'd accept them in this window in
> > the first place. richacl stuff isn't there as well, and I think that one
> > is clear "leave it for 4.5" fodder.
>
> So could you please remove the 4.5 stuff from your for-next branch
> until after the merge window closes.

Done.

> Also, I noticed these new warnings today:
>
> fs/orangefs/xattr.c:509:9: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
> .get = pvfs2_xattr_get_trusted,
> ^
> fs/orangefs/xattr.c:509:9: note: (near initialization for 'pvfs2_xattr_trusted_handler.get')
> fs/orangefs/xattr.c:510:9: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
> .set = pvfs2_xattr_set_trusted,
> ^
> fs/orangefs/xattr.c:510:9: note: (near initialization for 'pvfs2_xattr_trusted_handler.set')
> fs/orangefs/xattr.c:520:9: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
> .get = pvfs2_xattr_get_default,
> ^
> fs/orangefs/xattr.c:520:9: note: (near initialization for 'pvfs2_xattr_default_handler.get')
> fs/orangefs/xattr.c:521:9: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
> .set = pvfs2_xattr_set_default,
> ^
> fs/orangefs/xattr.c:521:9: note: (near initialization for 'pvfs2_xattr_default_handler.set')

That's "xattr handlers: Pass handler to operations instead of flags" fallout,
trivially adjusted (typical change is
-ext2_xattr_security_list(struct dentry *dentry, char *list, size_t list_size,
- const char *name, size_t name_len, int type)
+ext2_xattr_security_list(const struct xattr_handler *handler,
+ struct dentry *dentry, char *list, size_t list_size,
+ const char *name, size_t name_len)
with type replaced with handler->flags if it's used anywhere in the body;
AFAICS, none of orangefs instances use it at all, so it's just a matter of
changing the argument lists in pvfs2_xattr_[gs]et_{default,trusted},
adding const struct xattr_handler *handler in the beginning and removing
the last argument; callers in pvfs2_ioctl() should simply use
pvfs2_inode_[gs]etxattr()).

Note, however, that orangefs in linux-next lacks a lot of fixes (see
vfs.git#orangefs-untested for some; AFAICS, those are missing from all
branches in orangefs git tree) and there are problems I don't know
how to fix, mostly due to the lack of documentation. The last I've
heard from them was that they were putting such docs together; hopefully
once that get done we'll be able to sort the rest of that thing out.
It'll be after -rc1, though.

So xattr conflicts are the least of the problems there; those are easy
to adjust for, there are more serious issues in the entire thing ;-/
BTW, while we are at it - pvfs2_listxattr() doesn't even validate
resp.listxattr.returned_count, so a bogus response from buggered
server will do really interesting things to the kernel.

I'll cook the minimal fixup for API change after I get some sleep and
send it your way, unless somebody gets there first...

2015-11-11 10:28:53

by Stephen Rothwell

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

Hi Al,

On Wed, 11 Nov 2015 10:19:48 +0000 Al Viro <[email protected]> wrote:
>
> On Wed, Nov 11, 2015 at 07:16:36PM +1100, Stephen Rothwell wrote:
> >
> > So could you please remove the 4.5 stuff from your for-next branch
> > until after the merge window closes.
>
> Done.

Thanks.

> > Also, I noticed these new warnings today:
> >
> I'll cook the minimal fixup for API change after I get some sleep and
> send it your way, unless somebody gets there first...

Thanks again.

--
Cheers,
Stephen Rothwell [email protected]

2015-11-11 16:25:22

by Mike Marshall

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

I'm the Orangefs guy...

If the orangefs warnings that people see because of what's in
linux-next is annoying, I could focus on quieting them down...

We've been focusing on code review and documentation ever
since our last big exchange with Al and Linus...

-Mike

On Wed, Nov 11, 2015 at 5:28 AM, Stephen Rothwell <[email protected]> wrote:
> Hi Al,
>
> On Wed, 11 Nov 2015 10:19:48 +0000 Al Viro <[email protected]> wrote:
>>
>> On Wed, Nov 11, 2015 at 07:16:36PM +1100, Stephen Rothwell wrote:
>> >
>> > So could you please remove the 4.5 stuff from your for-next branch
>> > until after the merge window closes.
>>
>> Done.
>
> Thanks.
>
>> > Also, I noticed these new warnings today:
>> >
>> I'll cook the minimal fixup for API change after I get some sleep and
>> send it your way, unless somebody gets there first...
>
> Thanks again.
>
> --
> Cheers,
> Stephen Rothwell [email protected]
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2015-11-11 16:33:49

[permalink] [raw]

Subject: Re: fs: out of bounds on stack in iov_iter_advance

On Wed, Nov 11, 2015 at 10:19:48AM +0000, Al Viro wrote:

> I'll cook the minimal fixup for API change after I get some sleep and
> send it your way, unless somebody gets there first...

This should do it - switches ->ioctl() to pvfs2_inode_[gs]etxattr() and
converts xattr_handler ->[gs]et() to new API.

Signed-off-by: Al Viro <[email protected]>
---
diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
index feb1764..3d6ffe0 100644
--- a/fs/orangefs/file.c
+++ b/fs/orangefs/file.c
@@ -793,11 +793,10 @@ static long pvfs2_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
*/
if (cmd == FS_IOC_GETFLAGS) {
val = 0;
- ret = pvfs2_xattr_get_default(file->f_path.dentry,
- "user.pvfs2.meta_hint",
- &val,
- sizeof(val),
- 0);
+ ret = pvfs2_inode_getxattr(file_inode(file),
+ PVFS2_XATTR_NAME_DEFAULT_PREFIX,
+ "user.pvfs2.meta_hint",
+ &val, sizeof(val));
if (ret < 0 && ret != -ENODATA)
return ret;
else if (ret == -ENODATA)
@@ -827,12 +826,10 @@ static long pvfs2_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
gossip_debug(GOSSIP_FILE_DEBUG,
"pvfs2_ioctl: FS_IOC_SETFLAGS: %llu\n",
(unsigned long long)val);
- ret = pvfs2_xattr_set_default(file->f_path.dentry,
- "user.pvfs2.meta_hint",
- &val,
- sizeof(val),
- 0,
- 0);
+ ret = pvfs2_inode_setxattr(file_inode(file),
+ PVFS2_XATTR_NAME_DEFAULT_PREFIX,
+ "user.pvfs2.meta_hint",
+ &val, sizeof(val), 0);
}

return ret;
diff --git a/fs/orangefs/pvfs2-kernel.h b/fs/orangefs/pvfs2-kernel.h
index 29b4a48..43339c6 100644
--- a/fs/orangefs/pvfs2-kernel.h
+++ b/fs/orangefs/pvfs2-kernel.h
@@ -237,19 +237,6 @@ extern const struct xattr_handler *pvfs2_xattr_handlers[];
extern struct posix_acl *pvfs2_get_acl(struct inode *inode, int type);
extern int pvfs2_set_acl(struct inode *inode, struct posix_acl *acl, int type);

-int pvfs2_xattr_set_default(struct dentry *dentry,
- const char *name,
- const void *buffer,
- size_t size,
- int flags,
- int handler_flags);
-
-int pvfs2_xattr_get_default(struct dentry *dentry,
- const char *name,
- void *buffer,
- size_t size,
- int handler_flags);
-
/*
* Redefine xtvec structure so that we could move helper functions out of
* the define
diff --git a/fs/orangefs/xattr.c b/fs/orangefs/xattr.c
index 227eaa4..b683daa 100644
--- a/fs/orangefs/xattr.c
+++ b/fs/orangefs/xattr.c
@@ -447,12 +447,12 @@ out_unlock:
return ret;
}

-int pvfs2_xattr_set_default(struct dentry *dentry,
- const char *name,
- const void *buffer,
- size_t size,
- int flags,
- int handler_flags)
+static int pvfs2_xattr_set_default(const struct xattr_handler *handler,
+ struct dentry *dentry,
+ const char *name,
+ const void *buffer,
+ size_t size,
+ int flags)
{
return pvfs2_inode_setxattr(dentry->d_inode,
PVFS2_XATTR_NAME_DEFAULT_PREFIX,
@@ -462,11 +462,11 @@ int pvfs2_xattr_set_default(struct dentry *dentry,
flags);
}

-int pvfs2_xattr_get_default(struct dentry *dentry,
- const char *name,
- void *buffer,
- size_t size,
- int handler_flags)
+static int pvfs2_xattr_get_default(const struct xattr_handler *handler,
+ struct dentry *dentry,
+ const char *name,
+ void *buffer,
+ size_t size)
{
return pvfs2_inode_getxattr(dentry->d_inode,
PVFS2_XATTR_NAME_DEFAULT_PREFIX,
@@ -476,12 +476,12 @@ int pvfs2_xattr_get_default(struct dentry *dentry,

}

-static int pvfs2_xattr_set_trusted(struct dentry *dentry,
- const char *name,
- const void *buffer,
- size_t size,
- int flags,
- int handler_flags)
+static int pvfs2_xattr_set_trusted(const struct xattr_handler *handler,
+ struct dentry *dentry,
+ const char *name,
+ const void *buffer,
+ size_t size,
+ int flags)
{
return pvfs2_inode_setxattr(dentry->d_inode,
PVFS2_XATTR_NAME_TRUSTED_PREFIX,
@@ -491,11 +491,11 @@ static int pvfs2_xattr_set_trusted(struct dentry *dentry,
flags);
}

-static int pvfs2_xattr_get_trusted(struct dentry *dentry,
- const char *name,
- void *buffer,
- size_t size,
- int handler_flags)
+static int pvfs2_xattr_get_trusted(const struct xattr_handler *handler,
+ struct dentry *dentry,
+ const char *name,
+ void *buffer,
+ size_t size)
{
return pvfs2_inode_getxattr(dentry->d_inode,
PVFS2_XATTR_NAME_TRUSTED_PREFIX,

2015-11-11 16:36:37