LinuxLists.cc - [GIT PULL] Please pull NFS client changes for Linux 4.13

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On Thu, Jul 13, 2017 at 2:16 PM, Anna Schumaker
<[email protected]> wrote:
>
> git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs-for-4.13-1

Btw, your key seems to have expired, and doing a refresh on it doesn't fix it.

I'm sure you've refreshed your key, but apparently that refresh hasn't
been percolated to the public keyservers?

Linus

2017-07-14 07:09:27

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On Thu, Jul 13, 2017 at 02:43:14PM -0700, Linus Torvalds wrote:
> On Thu, Jul 13, 2017 at 2:16 PM, Anna Schumaker
> <[email protected]> wrote:
> >
> > git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs-for-4.13-1
>
> Btw, your key seems to have expired, and doing a refresh on it doesn't fix it.
>
> I'm sure you've refreshed your key, but apparently that refresh hasn't
> been percolated to the public keyservers?

As someone who has run into an issue in that area recently: I manually
had to refresh and re-upload my signing subkey and not just the primary
key, which wa rather confusing and took a long time to sort out.

2017-07-14 11:33:20

by Anna Schumaker

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On 07/14/2017 03:09 AM, Christoph Hellwig wrote:
> On Thu, Jul 13, 2017 at 02:43:14PM -0700, Linus Torvalds wrote:
>> On Thu, Jul 13, 2017 at 2:16 PM, Anna Schumaker
>> <[email protected]> wrote:
>>>
>>> git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs-for-4.13-1
>>
>> Btw, your key seems to have expired, and doing a refresh on it doesn't fix it.
>>
>> I'm sure you've refreshed your key, but apparently that refresh hasn't
>> been percolated to the public keyservers?

I assumed I had refreshed it once git let me sign things again, but maybe I missed a step.

>
> As someone who has run into an issue in that area recently: I manually
> had to refresh and re-upload my signing subkey and not just the primary
> key, which wa rather confusing and took a long time to sort out.
>

I'll start here. Thanks for the tip!

Anna

2017-07-14 14:25:46

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On Thu, Jul 13, 2017 at 05:16:24PM -0400, Anna Schumaker wrote:
> Hi Linus,
>
> The following changes since commit 32c1431eea4881a6b17bd7c639315010aeefa452:
>
> Linux 4.12-rc5 (2017-06-11 16:48:20 -0700)
>
> are available in the git repository at:
>
> git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs-for-4.13-1
>
> for you to fetch changes up to b4f937cffa66b3d56eb8f586e620d0b223a281a3:
>
> NFS: Don't run wake_up_bit() when nobody is waiting... (2017-07-13 16:57:18 -0400)

Since this landed, I'm seeing this during boot..

==================================================================
BUG: KASAN: global-out-of-bounds in strscpy+0x4a/0x230
Read of size 8 at addr ffffffffb4eeaf20 by task nfsd/688

CPU: 0 PID: 688 Comm: nfsd Not tainted 4.12.0-firewall+ #14
Call Trace:
dump_stack+0x68/0x94
print_address_description+0x2c/0x270
? strscpy+0x4a/0x230
kasan_report+0x239/0x350
__asan_load8+0x55/0x90
strscpy+0x4a/0x230
__ip_map_lookup+0x85/0x150
? ip_map_init+0x50/0x50
? lock_acquire+0x270/0x270
svcauth_unix_set_client+0x9f3/0xdc0
? svcauth_unix_set_client+0x5/0xdc0
? unix_gid_parse+0x340/0x340
? kasan_kmalloc+0xbb/0xf0
? groups_alloc+0x29/0x80
? __kmalloc+0x13b/0x360
? groups_alloc+0x29/0x80
? groups_alloc+0x48/0x80
? svcauth_unix_accept+0x3a5/0x3c0
svc_set_client+0x50/0x60
svc_process+0x901/0x10b0
? svc_register+0x430/0x430
? __might_sleep+0x78/0xf0
? preempt_count_sub+0xaf/0x120
? __validate_process_creds+0x9e/0x160
nfsd+0x250/0x380
? nfsd+0x5/0x380
kthread+0x1ab/0x200
? nfsd_destroy+0x1f0/0x1f0
? __kthread_create_on_node+0x340/0x340
ret_from_fork+0x27/0x40

The buggy address belongs to the variable:
str__nfsd__trace_system_name+0x3a0/0x3e0

Memory state around the buggy address:
ffffffffb4eeae00: 00 00 00 01 fa fa fa fa 00 00 00 00 00 04 fa fa
ffffffffb4eeae80: fa fa fa fa 04 fa fa fa fa fa fa fa 04 fa fa fa
>ffffffffb4eeaf00: fa fa fa fa 05 fa fa fa fa fa fa fa 00 00 00 00
^
ffffffffb4eeaf80: 00 fa fa fa fa fa fa fa 00 00 05 fa fa fa fa fa
ffffffffb4eeb000: 00 03 fa fa fa fa fa fa 00 07 fa fa fa fa fa fa
==================================================================

2017-07-14 16:36:17

by J. Bruce Fields

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On Fri, Jul 14, 2017 at 10:25:43AM -0400, Dave Jones wrote:
> On Thu, Jul 13, 2017 at 05:16:24PM -0400, Anna Schumaker wrote:
> > Hi Linus,
> >
> > The following changes since commit 32c1431eea4881a6b17bd7c639315010aeefa452:
> >
> > Linux 4.12-rc5 (2017-06-11 16:48:20 -0700)
> >
> > are available in the git repository at:
> >
> > git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs-for-4.13-1
> >
> > for you to fetch changes up to b4f937cffa66b3d56eb8f586e620d0b223a281a3:
> >
> > NFS: Don't run wake_up_bit() when nobody is waiting... (2017-07-13 16:57:18 -0400)
>
> Since this landed, I'm seeing this during boot..

__ip_map_lookup does have a strcpy, and it looks like that can be
implemented in terms of strscpy.

Based on that backtrace, it should just be copying from
nfsd_program->pg_class, which is initialized to "nfsd" and never
changed.

I spent a few minutes trying to figure out the tracing macros that
define str__nfsd__trace_system_name+0x3a0/0x3e0 and gave up.

So I have no idea what's going on....

--b.

>
> ==================================================================
> BUG: KASAN: global-out-of-bounds in strscpy+0x4a/0x230
> Read of size 8 at addr ffffffffb4eeaf20 by task nfsd/688
>
> CPU: 0 PID: 688 Comm: nfsd Not tainted 4.12.0-firewall+ #14
> Call Trace:
> dump_stack+0x68/0x94
> print_address_description+0x2c/0x270
> ? strscpy+0x4a/0x230
> kasan_report+0x239/0x350
> __asan_load8+0x55/0x90
> strscpy+0x4a/0x230
> __ip_map_lookup+0x85/0x150
> ? ip_map_init+0x50/0x50
> ? lock_acquire+0x270/0x270
> svcauth_unix_set_client+0x9f3/0xdc0
> ? svcauth_unix_set_client+0x5/0xdc0
> ? unix_gid_parse+0x340/0x340
> ? kasan_kmalloc+0xbb/0xf0
> ? groups_alloc+0x29/0x80
> ? __kmalloc+0x13b/0x360
> ? groups_alloc+0x29/0x80
> ? groups_alloc+0x48/0x80
> ? svcauth_unix_accept+0x3a5/0x3c0
> svc_set_client+0x50/0x60
> svc_process+0x901/0x10b0
> ? svc_register+0x430/0x430
> ? __might_sleep+0x78/0xf0
> ? preempt_count_sub+0xaf/0x120
> ? __validate_process_creds+0x9e/0x160
> nfsd+0x250/0x380
> ? nfsd+0x5/0x380
> kthread+0x1ab/0x200
> ? nfsd_destroy+0x1f0/0x1f0
> ? __kthread_create_on_node+0x340/0x340
> ret_from_fork+0x27/0x40
>
> The buggy address belongs to the variable:
> str__nfsd__trace_system_name+0x3a0/0x3e0
>
> Memory state around the buggy address:
> ffffffffb4eeae00: 00 00 00 01 fa fa fa fa 00 00 00 00 00 04 fa fa
> ffffffffb4eeae80: fa fa fa fa 04 fa fa fa fa fa fa fa 04 fa fa fa
> >ffffffffb4eeaf00: fa fa fa fa 05 fa fa fa fa fa fa fa 00 00 00 00
> ^
> ffffffffb4eeaf80: 00 fa fa fa fa fa fa fa 00 00 05 fa fa fa fa fa
> ffffffffb4eeb000: 00 03 fa fa fa fa fa fa 00 07 fa fa fa fa fa fa
> ==================================================================

2017-07-14 19:05:04

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On Fri, Jul 14, 2017 at 7:25 AM, Dave Jones <[email protected]> wrote:
> On Thu, Jul 13, 2017 at 05:16:24PM -0400, Anna Schumaker wrote:
> >
> > git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs-for-4.13-1
>
> Since this landed, I'm seeing this during boot..
>
> ==================================================================
> BUG: KASAN: global-out-of-bounds in strscpy+0x4a/0x230
> Read of size 8 at addr ffffffffb4eeaf20 by task nfsd/688

Is KASAN aware that strscpy() does the word-at-a-time optimistic reads
of the sources?

The problem may be that the source is initialized from the global
string "nfsd", and KASAN may be unhappy abotu the fact that we read 8
bytes from a 5-byte string (four plus NUL) as we do the word-at-a-time
strscpy..

That said, we do check the size first (because we also *write* 8 bytes
at a time), so maybe KASAN shouldn't even need to care.

Hmm. it really looks to me like this is actually a compiler bug (I'm
using current gcc in F26, which is gcc-7.1.1 - I'm assuming DaveJ is
the same).

This is the source code in __ip_map_lookup:

struct ip_map ip;
.....
strcpy(ip.m_class, class);

and "m_class" is 8 bytes in size:

struct ip_map {
...
char m_class[8]; /* e.g. "nfsd" */
...

yet when I look at the generated code for __ip_map_lookup, I see

movl $32, %edx #,
movq %r13, %rsi # class,
leaq 48(%rax), %rdi #, tmp126
call strscpy #

what's the bug here? Look at that third argument - %rdx. It is
initialized to 32.

WTF?

The code to turn "strcpy()" into "strscpy()" should pick the *smaller*
of the two object sizes as the size argument. How the hell is that
size argument 32?

Am I missing something? DaveJ, do you see the same?

Linus

2017-07-14 19:45:25

by Andrey Ryabinin

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On 07/14/2017 10:05 PM, Linus Torvalds wrote:
> On Fri, Jul 14, 2017 at 7:25 AM, Dave Jones <[email protected]> wrote:
>> On Thu, Jul 13, 2017 at 05:16:24PM -0400, Anna Schumaker wrote:
>> >
>> > git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs-for-4.13-1
>>
>> Since this landed, I'm seeing this during boot..
>>
>> ==================================================================
>> BUG: KASAN: global-out-of-bounds in strscpy+0x4a/0x230
>> Read of size 8 at addr ffffffffb4eeaf20 by task nfsd/688
>
> Is KASAN aware that strscpy() does the word-at-a-time optimistic reads
> of the sources?
>

Nope.

> The problem may be that the source is initialized from the global
> string "nfsd", and KASAN may be unhappy abotu the fact that we read 8
> bytes from a 5-byte string (four plus NUL) as we do the word-at-a-time
> strscpy..
>

Right.

> That said, we do check the size first (because we also *write* 8 bytes
> at a time), so maybe KASAN shouldn't even need to care.
>

Perhaps we could fallback to unoptimzed copy for KASAN case by setting max = 0
in strscpy().

> Hmm. it really looks to me like this is actually a compiler bug (I'm
> using current gcc in F26, which is gcc-7.1.1 - I'm assuming DaveJ is
> the same).
>
> This is the source code in __ip_map_lookup:
>
> struct ip_map ip;
> .....
> strcpy(ip.m_class, class);
>
> and "m_class" is 8 bytes in size:
>
> struct ip_map {
> ...
> char m_class[8]; /* e.g. "nfsd" */
> ...
>
> yet when I look at the generated code for __ip_map_lookup, I see
>
> movl $32, %edx #,
> movq %r13, %rsi # class,
> leaq 48(%rax), %rdi #, tmp126
> call strscpy #
>
> what's the bug here? Look at that third argume8nt - %rdx. It is
> initialized to 32.
>
> WTF?
>

It's not a compiler bug, it's a bug in our strcpy().
Whoever wrote this strcpy() into strscpy() code apparently didn't read carefully
enough gcc manual about __builtin_object_size().

Summary from https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html :

__builtin_object_size(ptr, type) returns a constant number of bytes from 'ptr' to the end of the object 'ptr'
pointer points to. "type" is an integer constant from 0 to 3. If the least significant bit is clear, objects
are whole variables, if it is set, a closest surrounding subobject is considered the object a pointer points to.
The second bit determines if maximum or minimum of remaining bytes is computed.

We have type = 0 in strcpy(), so the least significant bit is clear. So the 'ptr' is considered as a pointer to the whole
variable i.e. pointer to struct ip_map ip;
And the number of bytes from 'ip.m_class' to the end of the ip object is exactly 32.

I suppose that changing the type to 1 should fix this bug.

> The code to turn "strcpy()" into "strscpy()" should pick the *smaller*
> of the two object sizes as the size argument. How the hell is that
> size argument 32?
>
> Am I missing something? DaveJ, do you see the same?
>
> Linus
>

2017-07-14 19:48:56

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On Fri, Jul 14, 2017 at 12:05:02PM -0700, Linus Torvalds wrote:
> On Fri, Jul 14, 2017 at 7:25 AM, Dave Jones <[email protected]> wrote:
> > On Thu, Jul 13, 2017 at 05:16:24PM -0400, Anna Schumaker wrote:
> > >
> > > git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs-for-4.13-1
> >
> > Since this landed, I'm seeing this during boot..
> >
> > ==================================================================
> > BUG: KASAN: global-out-of-bounds in strscpy+0x4a/0x230
> > Read of size 8 at addr ffffffffb4eeaf20 by task nfsd/688
>
> Is KASAN aware that strscpy() does the word-at-a-time optimistic reads
> of the sources?
>
> The problem may be that the source is initialized from the global
> string "nfsd", and KASAN may be unhappy abotu the fact that we read 8
> bytes from a 5-byte string (four plus NUL) as we do the word-at-a-time
> strscpy..
>
> That said, we do check the size first (because we also *write* 8 bytes
> at a time), so maybe KASAN shouldn't even need to care.
>
> Hmm. it really looks to me like this is actually a compiler bug (I'm
> using current gcc in F26, which is gcc-7.1.1 - I'm assuming DaveJ is
> the same).

Debian's 6.4.0

> This is the source code in __ip_map_lookup:
>
> struct ip_map ip;
> .....
> strcpy(ip.m_class, class);
>
> and "m_class" is 8 bytes in size:
>
> struct ip_map {
> ...
> char m_class[8]; /* e.g. "nfsd" */
> ...
>
> yet when I look at the generated code for __ip_map_lookup, I see
>
> movl $32, %edx #,
> movq %r13, %rsi # class,
> leaq 48(%rax), %rdi #, tmp126
> call strscpy #
>
> what's the bug here? Look at that third argument - %rdx. It is
> initialized to 32.
>
> WTF?
>
> The code to turn "strcpy()" into "strscpy()" should pick the *smaller*
> of the two object sizes as the size argument. How the hell is that
> size argument 32?
>
> Am I missing something? DaveJ, do you see the same?

My compiler seems to have replaced the call with an inlined copy afaics.

0000000000000be0 <__ip_map_lookup>:
{
be0: e8 00 00 00 00 callq be5 <__ip_map_lookup+0x5>
be5: 55 push %rbp
be6: 48 b8 00 00 00 00 00 movabs $0xdffffc0000000000,%rax
bed: fc ff df
bf0: 48 89 e5 mov %rsp,%rbp
bf3: 41 57 push %r15
bf5: 41 56 push %r14
bf7: 4c 8d 32 lea (%rdx),%r14
if (strscpy(p, q, p_size < q_size ? p_size : q_size) < 0)
bfa: ba 20 00 00 00 mov $0x20,%edx
bff: 41 55 push %r13
c01: 4c 8d 2e lea (%rsi),%r13
c04: 41 54 push %r12
c06: 53 push %rbx
c07: 48 8d 1f lea (%rdi),%rbx
c0a: 48 8d a4 24 60 ff ff lea -0xa0(%rsp),%rsp
c11: ff
c12: 49 89 e4 mov %rsp,%r12
c15: 49 c1 ec 03 shr $0x3,%r12
c19: 48 c7 04 24 b3 8a b5 movq $0x41b58ab3,(%rsp)
c20: 41
c21: 48 c7 44 24 08 00 00 movq $0x0,0x8(%rsp)
c28: 00 00
c2a: 48 c7 44 24 10 00 00 movq $0x0,0x10(%rsp)
c31: 00 00
c33: 48 8d 7c 24 50 lea 0x50(%rsp),%rdi
c38: 4d 8d 24 04 lea (%r12,%rax,1),%r12
c3c: 41 c7 04 24 f1 f1 f1 movl $0xf1f1f1f1,(%r12)
c43: f1
c44: 41 c7 44 24 0c 00 00 movl $0xf4f40000,0xc(%r12)
c4b: f4 f4
c4d: 65 48 8b 04 25 28 00 mov %gs:0x28,%rax
c54: 00 00
c56: 48 89 84 24 98 00 00 mov %rax,0x98(%rsp)
c5d: 00
c5e: 31 c0 xor %eax,%eax
c60: e8 00 00 00 00 callq c65 <__ip_map_lookup+0x85>
c65: 48 85 c0 test %rax,%rax
c68: 0f 88 a0 00 00 00 js d0e <__ip_map_lookup+0x12e>
ip.m_addr = *addr;
c6e: be 10 00 00 00 mov $0x10,%esi
c73: 49 8d 3e lea (%r14),%rdi
c76: e8 00 00 00 00 callq c7b <__ip_map_lookup+0x9b>
c7b: 49 8b 56 08 mov 0x8(%r14),%rdx

But that mov $0x20,%edx looks like it might be the same value we're talking about.

Dave

2017-07-14 19:58:54

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On Fri, Jul 14, 2017 at 12:43 PM, Andrey Ryabinin
<[email protected]> wrote:
>
>> yet when I look at the generated code for __ip_map_lookup, I see
>>
>> movl $32, %edx #,
>> movq %r13, %rsi # class,
>> leaq 48(%rax), %rdi #, tmp126
>> call strscpy #
>>
>> what's the bug here? Look at that third argume8nt - %rdx. It is
>> initialized to 32.
>
> It's not a compiler bug, it's a bug in our strcpy().
> Whoever wrote this strcpy() into strscpy() code apparently didn't read carefully
> enough gcc manual about __builtin_object_size().
>
> Summary from https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html :
>
> __builtin_object_size(ptr, type) returns a constant number of bytes from 'ptr' to the end of the object 'ptr'
> pointer points to. "type" is an integer constant from 0 to 3. If the least significant bit is clear, objects
> are whole variables, if it is set, a closest surrounding subobject is considered the object a pointer points to.
> The second bit determines if maximum or minimum of remaining bytes is computed.
>
> We have type = 0 in strcpy(), so the least significant bit is clear. So the 'ptr' is considered as a pointer to the whole
> variable i.e. pointer to struct ip_map ip;
> And the number of bytes from 'ip.m_class' to the end of the ip object is exactly 32.
>
> I suppose that changing the type to 1 should fix this bug.

Oh, that absolutely needs to be done.

Because that "strcpy() -> strscpy()" conversion really depends on that
size being the right size (well, in this case minimal safe size) for
the actual accesses, exactly because "strscpy()" is perfectly willing
to write *past* the end of the destination string within that given
size limit (ie it reads and writes in the same 8-byte chunks).

So if you have a small target string that is contained in a big
object, then the "hardened" strcpy() code can actually end up
overwriting things past the end of the strring, even if the string
itself were to have fit in the buffer.

I note that every single use in string.h is buggy, and it worries me
that __compiletime_object_size() does this too. The only user of that
seems to be check_copy_size(), and now I'm a bit worried what that bug
may have hidden.

I find "hardening" code that adds bugs to be particularly bad and
ugly, the same way that I absolutely *hate* debugging code that turns
out to make debugging impossible (we had that with the "better" stack
tracing code that caused kernel panics to kill the machine entirely
rather than show the backtrace, and I'm still bitter about it a decade
after the fact).

There is something actively *evil* about it. Daniel, Kees, please jump on this.

Andrey, thanks for noticing this thing,

Linus

2017-07-14 20:28:30

by Andrey Ryabinin

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On 07/14/2017 10:58 PM, Linus Torvalds wrote:
> On Fri, Jul 14, 2017 at 12:43 PM, Andrey Ryabinin
> <[email protected]> wrote:
>>
>>> yet when I look at the generated code for __ip_map_lookup, I see
>>>
>>> movl $32, %edx #,
>>> movq %r13, %rsi # class,
>>> leaq 48(%rax), %rdi #, tmp126
>>> call strscpy #
>>>
>>> what's the bug here? Look at that third argume8nt - %rdx. It is
>>> initialized to 32.
>>
>> It's not a compiler bug, it's a bug in our strcpy().
>> Whoever wrote this strcpy() into strscpy() code apparently didn't read carefully
>> enough gcc manual about __builtin_object_size().
>>
>> Summary from https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html :
>>
>> __builtin_object_size(ptr, type) returns a constant number of bytes from 'ptr' to the end of the object 'ptr'
>> pointer points to. "type" is an integer constant from 0 to 3. If the least significant bit is clear, objects
>> are whole variables, if it is set, a closest surrounding subobject is considered the object a pointer points to.
>> The second bit determines if maximum or minimum of remaining bytes is computed.
>>
>> We have type = 0 in strcpy(), so the least significant bit is clear. So the 'ptr' is considered as a pointer to the whole
>> variable i.e. pointer to struct ip_map ip;
>> And the number of bytes from 'ip.m_class' to the end of the ip object is exactly 32.
>>
>> I suppose that changing the type to 1 should fix this bug.
>
> Oh, that absolutely needs to be done.
>
> Because that "strcpy() -> strscpy()" conversion really depends on that
> size being the right size (well, in this case minimal safe size) for
> the actual accesses, exactly because "strscpy()" is perfectly willing
> to write *past* the end of the destination string within that given
> size limit (ie it reads and writes in the same 8-byte chunks).
>
> So if you have a small target string that is contained in a big
> object, then the "hardened" strcpy() code can actually end up
> overwriting things past the end of the strring, even if the string
> itself were to have fit in the buffer.
>
> I note that every single use in string.h is buggy, and it worries me
> that __compiletime_object_size() does this too. The only user of that
> seems to be check_copy_size(), and now I'm a bit worried what that bug
> may have hidden.
>
> I find "hardening" code that adds bugs to be particularly bad and
> ugly, the same way that I absolutely *hate* debugging code that turns
> out to make debugging impossible (we had that with the "better" stack
> tracing code that caused kernel panics to kill the machine entirely
> rather than show the backtrace, and I'm still bitter about it a decade
> after the fact).
>

A have some more news to make you even more "happier" :)
strcpy() choose to copy 32-bytes instead of smaller 5-bytes because it has one more bug :)

GCC couldn't determine size of class (which is 5-byte string):
strcpy(ip.m_class, class);

So, p_size = 32 and q_size = -1, this "if (p_size == (size_t)-1 && q_size == (size_t)-1)" is false
(because of bogus '&&', obviously we should have '||' here)

and since (32 < (size_t)-1)
if (strscpy(p, q, p_size < q_size ? p_size : q_size) < 0)

we end up with 32-bytes strscpy().

Enjoy :)

> There is something actively *evil* about it. Daniel, Kees, please jump on this.
>
> Andrey, thanks for noticing this thing,
>
> Linus
>

2017-07-14 20:38:38

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On Fri, 2017-07-14 at 12:58 -0700, Linus Torvalds wrote:
> On Fri, Jul 14, 2017 at 12:43 PM, Andrey Ryabinin
> <[email protected]> wrote:
> >
> > > yet when I look at the generated code for __ip_map_lookup, I see
> > >
> > > movl $32, %edx #,
> > > movq %r13, %rsi # class,
> > > leaq 48(%rax), %rdi #, tmp126
> > > call strscpy #
> > >
> > > what's the bug here? Look at that third argume8nt - %rdx. It is
> > > initialized to 32.
> >
> > It's not a compiler bug, it's a bug in our strcpy().
> > Whoever wrote this strcpy() into strscpy() code apparently didn't
> > read carefully
> > enough gcc manual about __builtin_object_size().
> >
> > Summary from https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking
> > .html :
> >
> > __builtin_object_size(ptr, type) returns a constant number
> > of bytes from 'ptr' to the end of the object 'ptr'
> > pointer points to. "type" is an integer constant from 0 to
> > 3. If the least significant bit is clear, objects
> > are whole variables, if it is set, a closest surrounding
> > subobject is considered the object a pointer points to.
> > The second bit determines if maximum or minimum of remaining
> > bytes is computed.
> >
> > We have type = 0 in strcpy(), so the least significant bit is clear.
> > So the 'ptr' is considered as a pointer to the whole
> > variable i.e. pointer to struct ip_map ip;
> > And the number of bytes from 'ip.m_class' to the end of the ip
> > object is exactly 32.
> >
> > I suppose that changing the type to 1 should fix this bug.
>
> Oh, that absolutely needs to be done.
>
> Because that "strcpy() -> strscpy()" conversion really depends on that
> size being the right size (well, in this case minimal safe size) for
> the actual accesses, exactly because "strscpy()" is perfectly willing
> to write *past* the end of the destination string within that given
> size limit (ie it reads and writes in the same 8-byte chunks).
>
> So if you have a small target string that is contained in a big
> object, then the "hardened" strcpy() code can actually end up
> overwriting things past the end of the strring, even if the string
> itself were to have fit in the buffer.
>
> I note that every single use in string.h is buggy, and it worries me
> that __compiletime_object_size() does this too. The only user of that
> seems to be check_copy_size(), and now I'm a bit worried what that bug
> may have hidden.
>
> I find "hardening" code that adds bugs to be particularly bad and
> ugly, the same way that I absolutely *hate* debugging code that turns
> out to make debugging impossible (we had that with the "better" stack
> tracing code that caused kernel panics to kill the machine entirely
> rather than show the backtrace, and I'm still bitter about it a decade
> after the fact).
>
> There is something actively *evil* about it. Daniel, Kees, please jump
> on this.
>
> Andrey, thanks for noticing this thing,
>
> Linus

The issue is the usage of strscpy then, not the __builtin_object_size
type parameter. The type is set 0 rather than 1 to be more lenient by
not detecting intra-object overflow, which is going to come later.

If strscpy treats the count parameter as a *guarantee* of the dest size
rather than a limit, it's wrong to use it there, whether or not the type
parameter for __builtin_object_size is 0 or 1 since it can still return
a larger size. It's a limit with no guaranteed minimum.

My initial patch used strlcpy there, because I wasn't aware of strscpy
before it was suggested:

http://www.openwall.com/lists/kernel-hardening/2017/05/04/11

I was wrong to move it to strscpy. It could be switched back to strlcpy
again unless the kernel considers the count parameter to be a guarantee
that could be leveraged in the future. Using the fortified strlen +
memcpy would provide the improvement that strscpy was meant to provide
there over strlcpy.

2017-07-14 20:50:07

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On Fri, Jul 14, 2017 at 1:38 PM, Daniel Micay <[email protected]> wrote:
>
> If strscpy treats the count parameter as a *guarantee* of the dest size
> rather than a limit,

No, it's a *limit*.

And by a *limit*, I mean that we know that we can access both source
and destination within that limit.

> My initial patch used strlcpy there, because I wasn't aware of strscpy
> before it was suggested:

Since I'm looking at this, I note that the "strlcpy()" code is
complete garbage too, and has that same

p_size == (size_t)-1 && q_size == (size_t)-1

check which is wrong. Of course, in strlcpy, q_size is never actually
*used*, so the whole check seems bogus.

But no, strlcpy() is complete garbage, and should never be used. It is
truly a shit interface, and anybody who uses it is by definition
buggy.

Why? Because the return value of "strlcpy()" is defined to be ignoring
the limit, so you FUNDAMENTALLY must not use that thing on untrusted
source strings.

But since the whole *point* of people using it is for untrusted
sources, it by definition is garbage.

Ergo: don't use strlcpy(). It's unbelievable crap. It's wrong. There's
a reason we defined "strscpy()" as the way to do safe copies
(strncpy(), of course, is broken for both lack of NUL termination
_and_ for excessive NUL termination when a NUL did exist).

So quite frankly, this hardening code needs to be looked at again. And
no, if it uses "strlcpy()", then it's not hardering, it's just a pile
of crap.

Yes, I'm annoyed. I really get very very annoyed by "hardening" code
that does nothing of the kind.

Linus

2017-07-14 20:50:32

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

> My initial patch used strlcpy there, because I wasn't aware of strscpy
> before it was suggested:
>
> http://www.openwall.com/lists/kernel-hardening/2017/05/04/11
>
> I was wrong to move it to strscpy. It could be switched back to
> strlcpy
> again unless the kernel considers the count parameter to be a
> guarantee
> that could be leveraged in the future. Using the fortified strlen +
> memcpy would provide the improvement that strscpy was meant to provide
> there over strlcpy.

Similarly, the FORTIFY_SOURCE strcat uses strlcat with the assumption
that the count parameter is a limit, not a guarantee for a optimization.
There's only a C implementation and it currently doesn't, but if it's
meant to be a guarantee then the strcat needs to be changed too.

I'll make a fix moving away both from the existing functions.

2017-07-14 21:01:57

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On Fri, 2017-07-14 at 13:50 -0700, Linus Torvalds wrote:
> On Fri, Jul 14, 2017 at 1:38 PM, Daniel Micay <[email protected]>
> wrote:
> >
> > If strscpy treats the count parameter as a *guarantee* of the dest
> > size
> > rather than a limit,
>
> No, it's a *limit*.
>
> And by a *limit*, I mean that we know that we can access both source
> and destination within that limit.

FORTIFY_SOURCE needs to be able to pass a limit without implying that
there's a minimum. That's the distinction I was trying to make. It's
wrong to use anything where it's interpreted as a minimum here. Using
__builtin_object_size(ptr, 0) vs. __builtin_object_size(ptr, 1) doesn't
avoid the problem. __builtin_object_size(ptr, 1) returns a maximum among
the possible buffer sizes just like 0. It's just stricter, i.e. catches
intra-object overflow, which isn't desirable for the first take since it
will cause compatibility issues. There's code using memcpy,
copy_to_user, etc. to read / write multiple fields with a pointer to the
first one passed as the source / destination.

> > My initial patch used strlcpy there, because I wasn't aware of
> > strscpy
> > before it was suggested:
>
> Since I'm looking at this, I note that the "strlcpy()" code is
> complete garbage too, and has that same
>
> p_size == (size_t)-1 && q_size == (size_t)-1
>
> check which is wrong. Of course, in strlcpy, q_size is never actually
> *used*, so the whole check seems bogus.

That check is only an optimization. __builtin_object_size always returns
a constant, and it's (size_t)-1 when no limit could be found.

The reason q_size isn't used is because it doesn't yet prevent read
overflow. The commit message mentions that among the current limitations
along with __builtin_object_size(ptr, 1).

> But no, strlcpy() is complete garbage, and should never be used. It is
> truly a shit interface, and anybody who uses it is by definition
> buggy.
>
> Why? Because the return value of "strlcpy()" is defined to be ignoring
> the limit, so you FUNDAMENTALLY must not use that thing on untrusted
> source strings.
>
> But since the whole *point* of people using it is for untrusted
> sources, it by definition is garbage.
>
> Ergo: don't use strlcpy(). It's unbelievable crap. It's wrong. There's
> a reason we defined "strscpy()" as the way to do safe copies
> (strncpy(), of course, is broken for both lack of NUL termination
> _and_ for excessive NUL termination when a NUL did exist).

Sure, it doesn't prevent read overflow (but it's not worse than strcpy,
which is the purpose) which is why I said this:

"The fortified string functions should place a limit on reads from the
source. For strcat/strcpy, these could just be a variant of strlcat /
strlcpy using the size limit as a bound on both the source and
destination, with the return value only reporting whether truncation
occurred rather than providing the source length. It would be an easier
API for developers to use too and not that it really matters but it
would be more efficient for the case where truncation is intended."

That's why strscpy was suggested and I switched to that + updated the
commit message to only mention strcat, but it's wrong to use it here
because __builtin_object_size(p, 0) / __builtin_object_size(p, 1) are
only a guaranteed maximum length with no minimum guarantee.

2017-07-14 21:05:18

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

> The reason q_size isn't used is because it doesn't yet prevent read
> overflow. The commit message mentions that among the current
> limitations
> along with __builtin_object_size(ptr, 1).

Er rather, in strlcat, the q_size is unused after the fast path is
because strnlen obtains the constant again itself.

2017-07-14 23:59:04

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

> I find "hardening" code that adds bugs to be particularly bad and
> ugly, the same way that I absolutely *hate* debugging code that turns
> out to make debugging impossible (we had that with the "better" stack
> tracing code that caused kernel panics to kill the machine entirely
> rather than show the backtrace, and I'm still bitter about it a decade
> after the fact).

Agree, it's very important for this code to be correct and the string
functions have some subtleties so it needs scrutiny. I messed up strcpy
between v1 and v2 trying to add a proper read overflow check. My fault
for not looking more closely at strscpy before adopting it based on my
misinterpretation of the API.

This is primarily a bug finding feature right now and it has gotten a
few fixed that actually matter (most were unimportant memcpy read past
end of string constant but not all). I don't think it has another bug
like this strscpy misuse itself, but there will need to be some more
fixes for minor read overflows, etc. elsewhere in the tree before it'll
actually make sense as a hardening feature because it can turn a benign
read overflow into a DoS via BUG(). I think it will be fine for 4.13,
but I definitely wouldn't propose 'default y' for a while, even if there
was no performance cost (and there is).

Fix for this issue is here in case anyone just looks only at this thread
(realized I should have passed send-email a reply id):

http://marc.info/?l=linux-fsdevel&m=150006772418003&w=2

2017-07-16 21:15:34

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On Fri, Jul 14, 2017 at 10:25:43AM -0400, Dave Jones wrote:
> On Thu, Jul 13, 2017 at 05:16:24PM -0400, Anna Schumaker wrote:
> > Hi Linus,
> >
> > The following changes since commit 32c1431eea4881a6b17bd7c639315010aeefa452:
> >
> > Linux 4.12-rc5 (2017-06-11 16:48:20 -0700)
> >
> > are available in the git repository at:
> >
> > git://git.linux-nfs.org/projects/anna/linux-nfs.git tags/nfs-for-4.13-1
> >
> > for you to fetch changes up to b4f937cffa66b3d56eb8f586e620d0b223a281a3:
> >
> > NFS: Don't run wake_up_bit() when nobody is waiting... (2017-07-13 16:57:18 -0400)
>
> Since this landed, I'm seeing this during boot..
>
> ==================================================================
> BUG: KASAN: global-out-of-bounds in strscpy+0x4a/0x230
> Read of size 8 at addr ffffffffb4eeaf20 by task nfsd/688

Now that this one got fixed, this one fell out instead..
Will dig deeper tomorrow.

==================================================================
BUG: KASAN: global-out-of-bounds in call_start+0x93/0x100
Read of size 8 at addr ffffffff8d582588 by task kworker/0:1/22

CPU: 0 PID: 22 Comm: kworker/0:1 Not tainted 4.13.0-rc1-firewall+ #1
Workqueue: rpciod rpc_async_schedule
Call Trace:
dump_stack+0x68/0x94
print_address_description+0x2c/0x270
? call_start+0x93/0x100
kasan_report+0x239/0x350
__asan_load8+0x55/0x90
call_start+0x93/0x100
? rpc_default_callback+0x10/0x10
? rpc_default_callback+0x10/0x10
__rpc_execute+0x170/0x740
? rpc_wake_up_queued_task+0x50/0x50
? __lock_is_held+0x9f/0x110
rpc_async_schedule+0x12/0x20
process_one_work+0x4ba/0xb10
? process_one_work+0x401/0xb10
? pwq_dec_nr_in_flight+0x120/0x120
worker_thread+0x91/0x670
? __sched_text_start+0x8/0x8
kthread+0x1ab/0x200
? process_one_work+0xb10/0xb10
? __kthread_create_on_node+0x340/0x340
ret_from_fork+0x27/0x40

The buggy address belongs to the variable:
nfs_cb_version+0x8/0x740

Memory state around the buggy address:
ffffffff8d582480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffffffff8d582500: 00 00 00 00 00 00 00 00 00 00 fa fa fa fa fa fa
>ffffffff8d582580: 00 fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
^
ffffffff8d582600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffffffff8d582680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
==================================================================

2017-07-16 22:57:32

by Trond Myklebust

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

SGkgRGF2ZSwNCg0KT24gU3VuLCAyMDE3LTA3LTE2IGF0IDE3OjE1IC0wNDAwLCBEYXZlIEpvbmVz
IHdyb3RlOg0KPiBPbiBGcmksIEp1bCAxNCwgMjAxNyBhdCAxMDoyNTo0M0FNIC0wNDAwLCBEYXZl
IEpvbmVzIHdyb3RlOg0KPiAgPiBPbiBUaHUsIEp1bCAxMywgMjAxNyBhdCAwNToxNjoyNFBNIC0w
NDAwLCBBbm5hIFNjaHVtYWtlciB3cm90ZToNCj4gID4gID4gSGkgTGludXMsDQo+ICA+ICA+IA0K
PiAgPiAgPiBUaGUgZm9sbG93aW5nIGNoYW5nZXMgc2luY2UgY29tbWl0DQo+IDMyYzE0MzFlZWE0
ODgxYTZiMTdiZDdjNjM5MzE1MDEwYWVlZmE0NTI6DQo+ICA+ICA+IA0KPiAgPiAgPiAgIExpbnV4
IDQuMTItcmM1ICgyMDE3LTA2LTExIDE2OjQ4OjIwIC0wNzAwKQ0KPiAgPiAgPiANCj4gID4gID4g
YXJlIGF2YWlsYWJsZSBpbiB0aGUgZ2l0IHJlcG9zaXRvcnkgYXQ6DQo+ICA+ICA+IA0KPiAgPiAg
PiAgIGdpdDovL2dpdC5saW51eC1uZnMub3JnL3Byb2plY3RzL2FubmEvbGludXgtbmZzLmdpdCB0
YWdzL25mcy0NCj4gZm9yLTQuMTMtMQ0KPiAgPiAgPiANCj4gID4gID4gZm9yIHlvdSB0byBmZXRj
aCBjaGFuZ2VzIHVwIHRvDQo+IGI0ZjkzN2NmZmE2NmIzZDU2ZWI4ZjU4NmU2MjBkMGIyMjNhMjgx
YTM6DQo+ICA+ICA+IA0KPiAgPiAgPiAgIE5GUzogRG9uJ3QgcnVuIHdha2VfdXBfYml0KCkgd2hl
biBub2JvZHkgaXMgd2FpdGluZy4uLiAoMjAxNy0NCj4gMDctMTMgMTY6NTc6MTggLTA0MDApDQo+
ICA+IA0KPiAgPiBTaW5jZSB0aGlzIGxhbmRlZCwgSSdtIHNlZWluZyB0aGlzIGR1cmluZyBib290
Li4NCj4gID4gDQo+ICA+ICA9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PQ0KPiA9DQo+ICA+ICBCVUc6IEtBU0FOOiBnbG9iYWwt
b3V0LW9mLWJvdW5kcyBpbiBzdHJzY3B5KzB4NGEvMHgyMzANCj4gID4gIFJlYWQgb2Ygc2l6ZSA4
IGF0IGFkZHIgZmZmZmZmZmZiNGVlYWYyMCBieSB0YXNrIG5mc2QvNjg4DQo+IA0KPiBOb3cgdGhh
dCB0aGlzIG9uZSBnb3QgZml4ZWQsIHRoaXMgb25lIGZlbGwgb3V0IGluc3RlYWQuLg0KPiBXaWxs
IGRpZyBkZWVwZXIgdG9tb3Jyb3cuDQo+IA0KPiA9PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0NCj4gQlVHOiBLQVNBTjogZ2xv
YmFsLW91dC1vZi1ib3VuZHMgaW4gY2FsbF9zdGFydCsweDkzLzB4MTAwDQo+IFJlYWQgb2Ygc2l6
ZSA4IGF0IGFkZHIgZmZmZmZmZmY4ZDU4MjU4OCBieSB0YXNrIGt3b3JrZXIvMDoxLzIyDQo+IA0K
PiBDUFU6IDAgUElEOiAyMiBDb21tOiBrd29ya2VyLzA6MSBOb3QgdGFpbnRlZCA0LjEzLjAtcmMx
LWZpcmV3YWxsKyAjMSANCj4gV29ya3F1ZXVlOiBycGNpb2QgcnBjX2FzeW5jX3NjaGVkdWxlDQo+
IENhbGwgVHJhY2U6DQo+ICBkdW1wX3N0YWNrKzB4NjgvMHg5NA0KPiAgcHJpbnRfYWRkcmVzc19k
ZXNjcmlwdGlvbisweDJjLzB4MjcwDQo+ICA/IGNhbGxfc3RhcnQrMHg5My8weDEwMA0KPiAga2Fz
YW5fcmVwb3J0KzB4MjM5LzB4MzUwDQo+ICBfX2FzYW5fbG9hZDgrMHg1NS8weDkwDQo+ICBjYWxs
X3N0YXJ0KzB4OTMvMHgxMDANCj4gID8gcnBjX2RlZmF1bHRfY2FsbGJhY2srMHgxMC8weDEwDQo+
ICA/IHJwY19kZWZhdWx0X2NhbGxiYWNrKzB4MTAvMHgxMA0KPiAgX19ycGNfZXhlY3V0ZSsweDE3
MC8weDc0MA0KPiAgPyBycGNfd2FrZV91cF9xdWV1ZWRfdGFzaysweDUwLzB4NTANCj4gID8gX19s
b2NrX2lzX2hlbGQrMHg5Zi8weDExMA0KPiAgcnBjX2FzeW5jX3NjaGVkdWxlKzB4MTIvMHgyMA0K
PiAgcHJvY2Vzc19vbmVfd29yaysweDRiYS8weGIxMA0KPiAgPyBwcm9jZXNzX29uZV93b3JrKzB4
NDAxLzB4YjEwDQo+ICA/IHB3cV9kZWNfbnJfaW5fZmxpZ2h0KzB4MTIwLzB4MTIwDQo+ICB3b3Jr
ZXJfdGhyZWFkKzB4OTEvMHg2NzANCj4gID8gX19zY2hlZF90ZXh0X3N0YXJ0KzB4OC8weDgNCj4g
IGt0aHJlYWQrMHgxYWIvMHgyMDANCj4gID8gcHJvY2Vzc19vbmVfd29yaysweGIxMC8weGIxMA0K
PiAgPyBfX2t0aHJlYWRfY3JlYXRlX29uX25vZGUrMHgzNDAvMHgzNDANCj4gIHJldF9mcm9tX2Zv
cmsrMHgyNy8weDQwDQo+IA0KPiBUaGUgYnVnZ3kgYWRkcmVzcyBiZWxvbmdzIHRvIHRoZSB2YXJp
YWJsZToNCj4gIG5mc19jYl92ZXJzaW9uKzB4OC8weDc0MA0KDQpEb2VzIHRoZSBmb2xsb3dpbmcg
cGF0Y2ggZml4IGl0Pw0KDQpDaGVlcnMNCiAgVHJvbmQNCg0KODwtLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLQ0KRnJvbSBiOTIzMGNkZmJiZWU5MDE3OGExMzE4ZDIwY2QzMzcz
ZmZiNzU4Nzg4IE1vbiBTZXAgMTcgMDA6MDA6MDAgMjAwMQ0KRnJvbTogVHJvbmQgTXlrbGVidXN0
IDx0cm9uZC5teWtsZWJ1c3RAcHJpbWFyeWRhdGEuY29tPg0KRGF0ZTogU3VuLCAxNiBKdWwgMjAx
NyAxODo1MjoxOCAtMDQwMA0KU3ViamVjdDogW1BBVENIXSBuZnNkOiBGaXggYSBtZW1vcnkgc2Ny
aWJibGUgaW4gdGhlIGNhbGxiYWNrIGNoYW5uZWwNCg0KVGhlIG9mZnNldCBvZiB0aGUgZW50cnkg
aW4gc3RydWN0IHJwY192ZXJzaW9uIGhhcyB0byBtYXRjaCB0aGUgdmVyc2lvbg0KbnVtYmVyLg0K
DQpSZXBvcnRlZC1ieTogRGF2ZSBKb25lcyA8ZGF2ZWpAY29kZW1vbmtleS5vcmcudWs+DQpGaXhl
czogMWM1ODc2ZGRiZGI0ICgic3VucnBjOiBtb3ZlIHBfY291bnQgb3V0IG9mIHN0cnVjdCBycGNf
cHJvY2luZm8iKQ0KU2lnbmVkLW9mZi1ieTogVHJvbmQgTXlrbGVidXN0IDx0cm9uZC5teWtsZWJ1
c3RAcHJpbWFyeWRhdGEuY29tPg0KLS0tDQogZnMvbmZzZC9uZnM0Y2FsbGJhY2suYyB8IDYgKysr
LS0tDQogMSBmaWxlIGNoYW5nZWQsIDMgaW5zZXJ0aW9ucygrKSwgMyBkZWxldGlvbnMoLSkNCg0K
ZGlmZiAtLWdpdCBhL2ZzL25mc2QvbmZzNGNhbGxiYWNrLmMgYi9mcy9uZnNkL25mczRjYWxsYmFj
ay5jDQppbmRleCBiNDUwODNjMGY5YWUuLjQ5YjBhOWU3ZmYxOCAxMDA2NDQNCi0tLSBhL2ZzL25m
c2QvbmZzNGNhbGxiYWNrLmMNCisrKyBiL2ZzL25mc2QvbmZzNGNhbGxiYWNrLmMNCkBAIC03MjAs
OCArNzIwLDggQEAgc3RhdGljIGNvbnN0IHN0cnVjdCBycGNfdmVyc2lvbiBuZnNfY2JfdmVyc2lv
bjQgPSB7DQogCS5jb3VudHMJCQk9IG5mczRfY2JfY291bnRzLA0KIH07DQogDQotc3RhdGljIGNv
bnN0IHN0cnVjdCBycGNfdmVyc2lvbiAqbmZzX2NiX3ZlcnNpb25bXSA9IHsNCi0JJm5mc19jYl92
ZXJzaW9uNCwNCitzdGF0aWMgY29uc3Qgc3RydWN0IHJwY192ZXJzaW9uICpuZnNfY2JfdmVyc2lv
blsyXSA9IHsNCisJWzFdID0gJm5mc19jYl92ZXJzaW9uNCwNCiB9Ow0KIA0KIHN0YXRpYyBjb25z
dCBzdHJ1Y3QgcnBjX3Byb2dyYW0gY2JfcHJvZ3JhbTsNCkBAIC03OTUsNyArNzk1LDcgQEAgc3Rh
dGljIGludCBzZXR1cF9jYWxsYmFja19jbGllbnQoc3RydWN0IG5mczRfY2xpZW50ICpjbHAsIHN0
cnVjdCBuZnM0X2NiX2Nvbm4gKmMNCiAJCS5zYWRkcmVzcwk9IChzdHJ1Y3Qgc29ja2FkZHIgKikg
JmNvbm4tPmNiX3NhZGRyLA0KIAkJLnRpbWVvdXQJPSAmdGltZXBhcm1zLA0KIAkJLnByb2dyYW0J
PSAmY2JfcHJvZ3JhbSwNCi0JCS52ZXJzaW9uCT0gMCwNCisJCS52ZXJzaW9uCT0gMSwNCiAJCS5m
bGFncwkJPSAoUlBDX0NMTlRfQ1JFQVRFX05PUElORyB8IFJQQ19DTE5UX0NSRUFURV9RVUlFVCks
DQogCX07DQogCXN0cnVjdCBycGNfY2xudCAqY2xpZW50Ow0KLS0gDQoyLjEzLjMNCg0KLS0gDQpU
cm9uZCBNeWtsZWJ1c3QNCkxpbnV4IE5GUyBjbGllbnQgbWFpbnRhaW5lciwgUHJpbWFyeURhdGEN
CnRyb25kLm15a2xlYnVzdEBwcmltYXJ5ZGF0YS5jb20NCg==

2017-07-17 03:05:07

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On Sun, Jul 16, 2017 at 10:57:27PM +0000, Trond Myklebust wrote:

> > BUG: KASAN: global-out-of-bounds in call_start+0x93/0x100
> > Read of size 8 at addr ffffffff8d582588 by task kworker/0:1/22
>
> Does the following patch fix it?

Yep, seems to do the trick!

Dave

2017-07-17 19:02:22

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On Sun, Jul 16, 2017 at 8:05 PM, [email protected]
<[email protected]> wrote:
> On Sun, Jul 16, 2017 at 10:57:27PM +0000, Trond Myklebust wrote:
>
> > > BUG: KASAN: global-out-of-bounds in call_start+0x93/0x100
> > > Read of size 8 at addr ffffffff8d582588 by task kworker/0:1/22
> >
> > Does the following patch fix it?
>
> Yep, seems to do the trick!

I'm assuming I'll get this fix through a future pull request, and am
not applying the patch as-is. Just FYI.

Linus

2017-07-18 14:20:58

by J. Bruce Fields

[permalink] [raw]

Subject: [GIT PULL] Please pull an nfsd bugfix for 4.13

Please pull:

git://linux-nfs.org/~bfields/linux.git tags/nfsd-4.13-1

One fix for a problem introduced in the most recent merge window and
found by Dave Jones and KASAN.

--b.

Trond Myklebust (1):
nfsd: Fix a memory scribble in the callback channel

fs/nfsd/nfs4callback.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

2017-07-31 15:43:27

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

Another NFSv4 KASAN splat, this time from rc3.

==================================================================
BUG: KASAN: use-after-free in nfs4_exchange_id_done+0x3d7/0x8e0 [nfsv4]
Read of size 8 at addr ffff8804508af528 by task kworker/2:1/34

CPU: 2 PID: 34 Comm: kworker/2:1 Not tainted 4.13.0-rc3-think+ #1
Workqueue: rpciod rpc_async_schedule [sunrpc]
Call Trace:
dump_stack+0x68/0xa1
print_address_description+0xd9/0x270
kasan_report+0x257/0x370
? nfs4_exchange_id_done+0x3d7/0x8e0 [nfsv4]
check_memory_region+0x13a/0x1a0
__asan_loadN+0xf/0x20
nfs4_exchange_id_done+0x3d7/0x8e0 [nfsv4]
? nfs4_exchange_id_release+0xb0/0xb0 [nfsv4]
rpc_exit_task+0x69/0x110 [sunrpc]
? rpc_destroy_wait_queue+0x20/0x20 [sunrpc]
? rpc_destroy_wait_queue+0x20/0x20 [sunrpc]
__rpc_execute+0x1a0/0x840 [sunrpc]
? rpc_wake_up_queued_task+0x50/0x50 [sunrpc]
? __lock_is_held+0x9a/0x100
? debug_lockdep_rcu_enabled.part.16+0x1a/0x30
rpc_async_schedule+0x12/0x20 [sunrpc]
process_one_work+0x4d5/0xa70
? flush_delayed_work+0x70/0x70
? lock_acquire+0xfc/0x220
worker_thread+0x88/0x630
? pci_mmcfg_check_reserved+0xc0/0xc0
kthread+0x1a6/0x1f0
? process_one_work+0xa70/0xa70
? kthread_create_on_node+0xc0/0xc0
ret_from_fork+0x27/0x40

Allocated by task 1:
save_stack_trace+0x1b/0x20
save_stack+0x46/0xd0
kasan_kmalloc+0xad/0xe0
kasan_slab_alloc+0x12/0x20
kmem_cache_alloc+0xe0/0x2f0
getname_flags+0x43/0x220
getname+0x12/0x20
do_sys_open+0x14c/0x2b0
SyS_open+0x1e/0x20
do_syscall_64+0xea/0x260
return_from_SYSCALL_64+0x0/0x7a

Freed by task 1:
save_stack_trace+0x1b/0x20
save_stack+0x46/0xd0
kasan_slab_free+0x72/0xc0
kmem_cache_free+0xa8/0x300
putname+0x80/0x90
do_sys_open+0x22f/0x2b0
SyS_open+0x1e/0x20
do_syscall_64+0xea/0x260
return_from_SYSCALL_64+0x0/0x7a

The buggy address belongs to the object at ffff8804508aeac0\x0a which belongs to the cache names_cache of size 4096
The buggy address is located 2664 bytes inside of\x0a 4096-byte region [ffff8804508aeac0, ffff8804508afac0)
The buggy address belongs to the page:
page:ffffea0011422a00 count:1 mapcount:0 mapping: (null) index:0x0
[CONT START] compound_mapcount: 0
flags: 0x8000000000008100(slab|head)
raw: 8000000000008100 0000000000000000 0000000000000000 0000000100070007
raw: ffffea00113d6020 ffffea001136e220 ffff8804664f8040 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
ffff8804508af400: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff8804508af480: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>ffff8804508af500: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff8804508af580: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff8804508af600: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================

2017-08-01 05:35:47

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On Mon, Jul 31, 2017 at 8:43 AM, [email protected]
<[email protected]> wrote:
> Another NFSv4 KASAN splat, this time from rc3.
>
> BUG: KASAN: use-after-free in nfs4_exchange_id_done+0x3d7/0x8e0 [nfsv4]

Ugh. It's really hard to tell what access that it - KASAN doesn't
actually give enough information. There's lots of 8-byte accesses
there in that function.

Any chance of getting the output from

./scripts/faddr2line vmlinux nfs4_exchange_id_done+0x3d7/0x8e0

or something? That would be extremely useful in general for
stacktraces, but it's doubly useful for KASAN because most *other*
stacktraces tend to have a very limited number of things that can warn
(ie there's one or two WARN_ON() calls in a function), but KASAN can
have tens or hundreds..

Linus

> Read of size 8 at addr ffff8804508af528 by task kworker/2:1/34
>
> CPU: 2 PID: 34 Comm: kworker/2:1 Not tainted 4.13.0-rc3-think+ #1
> Workqueue: rpciod rpc_async_schedule [sunrpc]
> Call Trace:
> dump_stack+0x68/0xa1
> print_address_description+0xd9/0x270
> kasan_report+0x257/0x370
> ? nfs4_exchange_id_done+0x3d7/0x8e0 [nfsv4]
> check_memory_region+0x13a/0x1a0
> __asan_loadN+0xf/0x20
> nfs4_exchange_id_done+0x3d7/0x8e0 [nfsv4]
> ? nfs4_exchange_id_release+0xb0/0xb0 [nfsv4]
> rpc_exit_task+0x69/0x110 [sunrpc]
> ? rpc_destroy_wait_queue+0x20/0x20 [sunrpc]
> ? rpc_destroy_wait_queue+0x20/0x20 [sunrpc]
> __rpc_execute+0x1a0/0x840 [sunrpc]
> ? rpc_wake_up_queued_task+0x50/0x50 [sunrpc]
> ? __lock_is_held+0x9a/0x100
> ? debug_lockdep_rcu_enabled.part.16+0x1a/0x30
> rpc_async_schedule+0x12/0x20 [sunrpc]
> process_one_work+0x4d5/0xa70
> ? flush_delayed_work+0x70/0x70
> ? lock_acquire+0xfc/0x220
> worker_thread+0x88/0x630
> ? pci_mmcfg_check_reserved+0xc0/0xc0
> kthread+0x1a6/0x1f0
> ? process_one_work+0xa70/0xa70
> ? kthread_create_on_node+0xc0/0xc0
> ret_from_fork+0x27/0x40
>
> Allocated by task 1:
> save_stack_trace+0x1b/0x20
> save_stack+0x46/0xd0
> kasan_kmalloc+0xad/0xe0
> kasan_slab_alloc+0x12/0x20
> kmem_cache_alloc+0xe0/0x2f0
> getname_flags+0x43/0x220
> getname+0x12/0x20
> do_sys_open+0x14c/0x2b0
> SyS_open+0x1e/0x20
> do_syscall_64+0xea/0x260
> return_from_SYSCALL_64+0x0/0x7a
>
> Freed by task 1:
> save_stack_trace+0x1b/0x20
> save_stack+0x46/0xd0
> kasan_slab_free+0x72/0xc0
> kmem_cache_free+0xa8/0x300
> putname+0x80/0x90
> do_sys_open+0x22f/0x2b0
> SyS_open+0x1e/0x20
> do_syscall_64+0xea/0x260
> return_from_SYSCALL_64+0x0/0x7a
>
> The buggy address belongs to the object at ffff8804508aeac0\x0a which belongs to the cache names_cache of size 4096
> The buggy address is located 2664 bytes inside of\x0a 4096-byte region [ffff8804508aeac0, ffff8804508afac0)
> The buggy address belongs to the page:
> page:ffffea0011422a00 count:1 mapcount:0 mapping: (null) index:0x0
> [CONT START] compound_mapcount: 0
> flags: 0x8000000000008100(slab|head)
> raw: 8000000000008100 0000000000000000 0000000000000000 0000000100070007
> raw: ffffea00113d6020 ffffea001136e220 ffff8804664f8040 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
> ffff8804508af400: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff8804508af480: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>>ffff8804508af500: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ^
> ffff8804508af580: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff8804508af600: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ==================================================================
>

2017-08-01 15:51:36

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On Mon, Jul 31, 2017 at 10:35:45PM -0700, Linus Torvalds wrote:
> On Mon, Jul 31, 2017 at 8:43 AM, [email protected]
> <[email protected]> wrote:
> > Another NFSv4 KASAN splat, this time from rc3.
> >
> > BUG: KASAN: use-after-free in nfs4_exchange_id_done+0x3d7/0x8e0 [nfsv4]
>
> Ugh. It's really hard to tell what access that it - KASAN doesn't
> actually give enough information. There's lots of 8-byte accesses
> there in that function.
>
> Any chance of getting the output from
>
> ./scripts/faddr2line vmlinux nfs4_exchange_id_done+0x3d7/0x8e0

Hm, that points to this..

7463 /* Save the EXCHANGE_ID verifier session trunk tests */
7464 memcpy(clp->cl_confirm.data, cdata->args.verifier->data,
7465 sizeof(clp->cl_confirm.data));

Dave

2017-08-01 17:20:33

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On Tue, Aug 1, 2017 at 8:51 AM, [email protected]
<[email protected]> wrote:
> On Mon, Jul 31, 2017 at 10:35:45PM -0700, Linus Torvalds wrote:
> > Any chance of getting the output from
> >
> > ./scripts/faddr2line vmlinux nfs4_exchange_id_done+0x3d7/0x8e0
>
>
> Hm, that points to this..
>
> 7463 /* Save the EXCHANGE_ID verifier session trunk tests */
> 7464 memcpy(clp->cl_confirm.data, cdata->args.verifier->data,
> 7465 sizeof(clp->cl_confirm.data));

Ok, that certainly made no sense to me, because the KASAN report made
it look like a stale pathname access (allocated in getname, freed in
putname), but I think the issue is more fundamental than that.

That cdata->args.verifier seems to be entirely broken. AT least for
the "xprt == NULL" case, it does the following:

- use the address of a local variable ("&verifier")

- wait for the rpc completion using rpc_wait_for_completion_task().

That's unacceptably buggy crap. rpc_wait_for_completion_task() will
happily exit on a deadly signal even if the rpc hasn't been completed,
so now you'll have a stale pointer to a stack that has been freed.

So I think the 'pathname' part may actually be entirely a red herring,
and it's the underlying access itself that just picks up a random
pointer from a stack that now contains something different. And KASAN
didn't notice the stale stack access itself, because the stack slot is
still valid - it's just no longer the original 'verifier' allocation.

Or *something* like that.

None of this looks even remotely new, though - the code seems to go
back to 2009. Have you just changed what you're testing to trigger
these things?

I'm not even sure why it does that stupid stack allocation. It does a
*real* allocation just a few lines later:

struct nfs41_exchange_id_data *calldata
...
calldata = kzalloc(sizeof(*calldata), GFP_NOFS);

and the whole verifier structure could easily have been part of that
same allocation as far as I can tell.

And that really might seem to be the right thing to do.

TOTALLY UNTESTED PROBABLY COMPLETE CRAP patch attatched.

That patch compiles for me. It *might* even work. Or it might just be
the ramblings of a diseased mind.

Anna? Trond?

So caveat probatorem,

Linus

Attachments:

patch.diff (1.86 kB)

2017-08-01 17:30:38

by Trond Myklebust

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

T24gVHVlLCAyMDE3LTA4LTAxIGF0IDEwOjIwIC0wNzAwLCBMaW51cyBUb3J2YWxkcyB3cm90ZToN
Cj4gT24gVHVlLCBBdWcgMSwgMjAxNyBhdCA4OjUxIEFNLCBkYXZlakBjb2RlbW9ua2V5Lm9yZy51
aw0KPiA8ZGF2ZWpAY29kZW1vbmtleS5vcmcudWs+IHdyb3RlOg0KPiA+IE9uIE1vbiwgSnVsIDMx
LCAyMDE3IGF0IDEwOjM1OjQ1UE0gLTA3MDAsIExpbnVzIFRvcnZhbGRzIHdyb3RlOg0KPiA+ICA+
IEFueSBjaGFuY2Ugb2YgZ2V0dGluZyB0aGUgb3V0cHV0IGZyb20NCj4gPiAgPg0KPiA+ICA+ICAg
IC4vc2NyaXB0cy9mYWRkcjJsaW5lIHZtbGludXgNCj4gPiBuZnM0X2V4Y2hhbmdlX2lkX2RvbmUr
MHgzZDcvMHg4ZTANCj4gPiANCj4gPiANCj4gPiBIbSwgdGhhdCBwb2ludHMgdG8gdGhpcy4uDQo+
ID4gDQo+ID4gNzQ2MyAgICAgICAgICAgICAgICAgLyogU2F2ZSB0aGUgRVhDSEFOR0VfSUQgdmVy
aWZpZXIgc2Vzc2lvbiB0cnVuaw0KPiA+IHRlc3RzICovDQo+ID4gNzQ2NCAgICAgICAgICAgICAg
ICAgbWVtY3B5KGNscC0+Y2xfY29uZmlybS5kYXRhLCBjZGF0YS0NCj4gPiA+YXJncy52ZXJpZmll
ci0+ZGF0YSwNCj4gPiA3NDY1ICAgICAgICAgICAgICAgICAgICAgICAgc2l6ZW9mKGNscC0+Y2xf
Y29uZmlybS5kYXRhKSk7DQo+IA0KPiBPaywgdGhhdCBjZXJ0YWlubHkgbWFkZSBubyBzZW5zZSB0
byBtZSwgYmVjYXVzZSB0aGUgS0FTQU4gcmVwb3J0IG1hZGUNCj4gaXQgbG9vayBsaWtlIGEgc3Rh
bGUgcGF0aG5hbWUgYWNjZXNzIChhbGxvY2F0ZWQgaW4gZ2V0bmFtZSwgZnJlZWQgaW4NCj4gcHV0
bmFtZSksIGJ1dCBJIHRoaW5rIHRoZSBpc3N1ZSBpcyBtb3JlIGZ1bmRhbWVudGFsIHRoYW4gdGhh
dC4NCj4gDQo+IFRoYXQgY2RhdGEtPmFyZ3MudmVyaWZpZXIgc2VlbXMgdG8gYmUgZW50aXJlbHkg
YnJva2VuLiBBVCBsZWFzdCBmb3INCj4gdGhlICJ4cHJ0ID09IE5VTEwiIGNhc2UsIGl0IGRvZXMg
dGhlIGZvbGxvd2luZzoNCj4gDQo+ICAtIHVzZSB0aGUgYWRkcmVzcyBvZiBhIGxvY2FsIHZhcmlh
YmxlICgiJnZlcmlmaWVyIikNCj4gDQo+ICAtIHdhaXQgZm9yIHRoZSBycGMgY29tcGxldGlvbiB1
c2luZyBycGNfd2FpdF9mb3JfY29tcGxldGlvbl90YXNrKCkuDQo+IA0KPiBUaGF0J3MgdW5hY2Nl
cHRhYmx5IGJ1Z2d5IGNyYXAuIHJwY193YWl0X2Zvcl9jb21wbGV0aW9uX3Rhc2soKSB3aWxsDQo+
IGhhcHBpbHkgZXhpdCBvbiBhIGRlYWRseSBzaWduYWwgZXZlbiBpZiB0aGUgcnBjIGhhc24ndCBi
ZWVuDQo+IGNvbXBsZXRlZCwNCj4gc28gbm93IHlvdSdsbCBoYXZlIGEgc3RhbGUgcG9pbnRlciB0
byBhIHN0YWNrIHRoYXQgaGFzIGJlZW4gZnJlZWQuDQo+IA0KPiBTbyBJIHRoaW5rIHRoZSAncGF0
aG5hbWUnIHBhcnQgbWF5IGFjdHVhbGx5IGJlIGVudGlyZWx5IGEgcmVkDQo+IGhlcnJpbmcsDQo+
IGFuZCBpdCdzIHRoZSB1bmRlcmx5aW5nIGFjY2VzcyBpdHNlbGYgdGhhdCBqdXN0IHBpY2tzIHVw
IGEgcmFuZG9tDQo+IHBvaW50ZXIgZnJvbSBhIHN0YWNrIHRoYXQgbm93IGNvbnRhaW5zIHNvbWV0
aGluZyBkaWZmZXJlbnQuIEFuZCBLQVNBTg0KPiBkaWRuJ3Qgbm90aWNlIHRoZSBzdGFsZSBzdGFj
ayBhY2Nlc3MgaXRzZWxmLCBiZWNhdXNlIHRoZSBzdGFjayBzbG90DQo+IGlzDQo+IHN0aWxsIHZh
bGlkIC0gaXQncyBqdXN0IG5vIGxvbmdlciB0aGUgb3JpZ2luYWwgJ3ZlcmlmaWVyJyBhbGxvY2F0
aW9uLg0KPiANCj4gT3IgKnNvbWV0aGluZyogbGlrZSB0aGF0Lg0KPiANCj4gTm9uZSBvZiB0aGlz
IGxvb2tzIGV2ZW4gcmVtb3RlbHkgbmV3LCB0aG91Z2ggLSB0aGUgY29kZSBzZWVtcyB0byBnbw0K
PiBiYWNrIHRvIDIwMDkuIEhhdmUgeW91IGp1c3QgY2hhbmdlZCB3aGF0IHlvdSdyZSB0ZXN0aW5n
IHRvIHRyaWdnZXINCj4gdGhlc2UgdGhpbmdzPw0KPiANCj4gSSdtIG5vdCBldmVuIHN1cmUgd2h5
IGl0IGRvZXMgdGhhdCBzdHVwaWQgc3RhY2sgYWxsb2NhdGlvbi4gSXQgZG9lcyBhDQo+ICpyZWFs
KiBhbGxvY2F0aW9uIGp1c3QgYSBmZXcgbGluZXMgbGF0ZXI6DQo+IA0KPiAgICAgICAgIHN0cnVj
dCBuZnM0MV9leGNoYW5nZV9pZF9kYXRhICpjYWxsZGF0YQ0KPiAgICAgICAgIC4uLg0KPiAgICAg
ICAgIGNhbGxkYXRhID0ga3phbGxvYyhzaXplb2YoKmNhbGxkYXRhKSwgR0ZQX05PRlMpOw0KPiAN
Cj4gYW5kIHRoZSB3aG9sZSB2ZXJpZmllciBzdHJ1Y3R1cmUgY291bGQgZWFzaWx5IGhhdmUgYmVl
biBwYXJ0IG9mIHRoYXQNCj4gc2FtZSBhbGxvY2F0aW9uIGFzIGZhciBhcyBJIGNhbiB0ZWxsLg0K
PiANCj4gQW5kIHRoYXQgcmVhbGx5IG1pZ2h0IHNlZW0gdG8gYmUgdGhlIHJpZ2h0IHRoaW5nIHRv
IGRvLg0KPiANCj4gVE9UQUxMWSBVTlRFU1RFRCBQUk9CQUJMWSBDT01QTEVURSBDUkFQIHBhdGNo
IGF0dGF0Y2hlZC4NCj4gDQo+IFRoYXQgcGF0Y2ggY29tcGlsZXMgZm9yIG1lLiBJdCAqbWlnaHQq
IGV2ZW4gd29yay4gT3IgaXQgbWlnaHQganVzdCBiZQ0KPiB0aGUgcmFtYmxpbmdzIG9mIGEgZGlz
ZWFzZWQgbWluZC4NCj4gDQo+IEFubmE/IFRyb25kPw0KPiANCg0KSSBjYW1lIHRvIHRoZSBzYW1l
IGNvbmNsdXNpb24geWVzdGVyZGF5LCBhbmQgaGF2ZSBhIHN0YWJsZSBwYXRjaCB0aGF0DQpkb2Vz
IHNvbWV0aGluZyBzaW1pbGFyLiBJIGp1c3QgZ290IGRpc3RyYWN0ZWQgd2l0aCB0aGUgb3RoZXIg
YnVncyB0aGF0DQp3ZXJlIGludHJvZHVjZWQgYnkgdGhlIGV4Y2hhbmdlaWQgcGF0Y2ggc2VyaWVz
IGluIExpbnV4LTQuOSAoaW5jbHVkaW5nDQp3aGF0IGxvb2tzIGxpa2UgYSBkdXBsaWNhdGUgZnJl
ZSBpc3N1ZSBpbiBuZnM0X3Rlc3Rfc2Vzc2lvbl90cnVuaygpKS4NCg0KSSBjYW4gcGFzcyBhIGZl
dyBvZiB0aGUgbW9yZSBjcml0aWNhbCBwYXRjaGVzIG9uIHRvIEFubmEgZm9yIG1lcmdpbmcgaW4N
CnRoaXMgY3ljbGUsIHRoZW4gSSd2ZSBnb3Qgc29tZSBjbGVhbiB1cHMgcmVhZHkgZm9yIHRoZSA0
LjE0IG1lcmdlDQp3aW5kb3cuDQoNCkNoZWVycw0KICBUcm9uZA0KDQotLSANClRyb25kIE15a2xl
YnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyLCBQcmltYXJ5RGF0YQ0KdHJvbmQubXlr
bGVidXN0QHByaW1hcnlkYXRhLmNvbQ0K

2017-08-01 17:50:33

[permalink] [raw]

Subject: Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

On Tue, Aug 01, 2017 at 10:20:31AM -0700, Linus Torvalds wrote:

> So I think the 'pathname' part may actually be entirely a red herring,
> and it's the underlying access itself that just picks up a random
> pointer from a stack that now contains something different. And KASAN
> didn't notice the stale stack access itself, because the stack slot is
> still valid - it's just no longer the original 'verifier' allocation.
>
> Or *something* like that.
>
> None of this looks even remotely new, though - the code seems to go
> back to 2009. Have you just changed what you're testing to trigger
> these things?

No idea why it only just showed up, but it isn't 100% reproducable
either. A month or so ago I did disable the V4 code on the server
completely (as I was using v3 everywhere else), so maybe I started hitting
a fallback path somewhere. *shrug*

Dave

2017-08-01 17:53:19