* fs: Uninitialized memory read at take_dentry_name_snapshot
@ 2017-09-04 12:12 Tetsuo Handa
2017-09-04 13:21 ` Al Viro
0 siblings, 1 reply; 6+ messages in thread
From: Tetsuo Handa @ 2017-09-04 12:12 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel
Hello.
I hit kmemcheck splat on commit 49d31c2f389acfe8 ("dentry name snapshots") using linux-next-20170901.
Changing to strncpy() fixes this problem, but using strncpy() only if CONFIG_KMEMCHECK=y is better
for performance?
[ 788.180175] WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (ffff8fa27465ac50)
[ 788.184248] 636f6e66696766732e746d70000000000010000000000000020000000188ffff
[ 788.186989] i i i i i i i i i i i i i u u u u u u u u u u i i i i i u u u u
[ 788.189841] ^
[ 788.191937] RIP: 0010:take_dentry_name_snapshot+0x28/0x50
[ 788.194225] RSP: 0018:ffffa83000f5bdf8 EFLAGS: 00010246
[ 788.196453] RAX: 0000000000000020 RBX: ffff8fa274b20550 RCX: 0000000000000002
[ 788.199200] RDX: ffffa83000f5be40 RSI: ffff8fa27465ac50 RDI: ffffa83000f5be60
[ 788.201950] RBP: ffffa83000f5bdf8 R08: ffffa83000f5be48 R09: 0000000000000001
[ 788.204773] R10: ffff8fa27465ac00 R11: ffff8fa27465acc0 R12: ffff8fa27465ac00
[ 788.207625] R13: ffff8fa27465acc0 R14: 0000000000000000 R15: 0000000000000000
[ 788.210399] FS: 00007f79737ac8c0(0000) GS:ffffffff8fc30000(0000) knlGS:0000000000000000
[ 788.213422] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 788.215811] CR2: ffff8fa274c0b000 CR3: 0000000134aa7002 CR4: 00000000000606f0
[ 788.218679] take_dentry_name_snapshot+0x28/0x50
[ 788.220915] vfs_rename+0x128/0x870
[ 788.222765] SyS_rename+0x3b2/0x3d0
[ 788.224549] entry_SYSCALL_64_fastpath+0x1a/0xa4
[ 788.226645] 0xffffffffffffffff
# ./scripts/faddr2line vmlinux take_dentry_name_snapshot+0x28/0x50
take_dentry_name_snapshot+0x28/0x50:
__inline_memcpy at arch/x86/include/asm/string_64.h:13
(inlined by) take_dentry_name_snapshot at fs/dcache.c:294
0000000000000330 <take_dentry_name_snapshot>:
330: 55 push %rbp
331: 48 89 fa mov %rdi,%rdx
334: 48 89 e5 mov %rsp,%rbp
337: 48 8b 46 28 mov 0x28(%rsi),%rax
33b: 48 83 c6 38 add $0x38,%rsi
33f: 48 39 f0 cmp %rsi,%rax
342: 75 26 jne 36a <take_dentry_name_snapshot+0x3a>
344: 4c 8d 47 08 lea 0x8(%rdi),%r8
348: 48 89 c6 mov %rax,%rsi
34b: b9 08 00 00 00 mov $0x8,%ecx
350: b8 20 00 00 00 mov $0x20,%eax
355: 4c 89 c7 mov %r8,%rdi
358: f3 a5 rep movsl %ds:(%rsi),%es:(%rdi) // <= take_dentry_name_snapshot+0x28/0x50
35a: a8 02 test $0x2,%al
35c: 74 02 je 360 <take_dentry_name_snapshot+0x30>
35e: 66 a5 movsw %ds:(%rsi),%es:(%rdi)
360: a8 01 test $0x1,%al
362: 74 01 je 365 <take_dentry_name_snapshot+0x35>
364: a4 movsb %ds:(%rsi),%es:(%rdi)
365: 4c 89 02 mov %r8,(%rdx)
368: 5d pop %rbp
369: c3 retq
36a: ff 40 f0 incl -0x10(%rax)
36d: 48 89 07 mov %rax,(%rdi)
370: 5d pop %rbp
371: c3 retq
372: 0f 1f 40 00 nopl 0x0(%rax)
376: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
37d: 00 00 00
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: fs: Uninitialized memory read at take_dentry_name_snapshot
2017-09-04 12:12 fs: Uninitialized memory read at take_dentry_name_snapshot Tetsuo Handa
@ 2017-09-04 13:21 ` Al Viro
2017-09-04 14:11 ` Tetsuo Handa
0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2017-09-04 13:21 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: linux-fsdevel
On Mon, Sep 04, 2017 at 09:12:38PM +0900, Tetsuo Handa wrote:
> Hello.
>
> I hit kmemcheck splat on commit 49d31c2f389acfe8 ("dentry name snapshots") using linux-next-20170901.
> Changing to strncpy() fixes this problem, but using strncpy() only if CONFIG_KMEMCHECK=y is better
> for performance?
*shrug*
If anything, use dentry->d_name.len + 1 instead of DNAME_INLINE_LEN there, but
that's really a false positive.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: fs: Uninitialized memory read at take_dentry_name_snapshot
2017-09-04 13:21 ` Al Viro
@ 2017-09-04 14:11 ` Tetsuo Handa
2017-09-04 14:22 ` Al Viro
0 siblings, 1 reply; 6+ messages in thread
From: Tetsuo Handa @ 2017-09-04 14:11 UTC (permalink / raw)
To: viro; +Cc: linux-fsdevel
Al Viro wrote:
> On Mon, Sep 04, 2017 at 09:12:38PM +0900, Tetsuo Handa wrote:
> > Hello.
> >
> > I hit kmemcheck splat on commit 49d31c2f389acfe8 ("dentry name snapshots") using linux-next-20170901.
> > Changing to strncpy() fixes this problem, but using strncpy() only if CONFIG_KMEMCHECK=y is better
> > for performance?
>
> *shrug*
>
> If anything, use dentry->d_name.len + 1 instead of DNAME_INLINE_LEN there, but
> that's really a false positive.
A false positive? Isn't it because we do
dentry = kmem_cache_alloc(dentry_cache, GFP_KERNEL);
(...snipped...)
dname = dentry->d_iname;
(...snipped...)
memcpy(dname, name->name, name->len);
dname[name->len] = 0;
at __d_alloc() which leaves a room for some of d_iname[] bytes uninitialized?
So, I think either pad explicitly at __f_alloc() or use dentry->d_name.len + 1 is needed.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: fs: Uninitialized memory read at take_dentry_name_snapshot
2017-09-04 14:11 ` Tetsuo Handa
@ 2017-09-04 14:22 ` Al Viro
2017-09-04 16:56 ` Vegard Nossum
0 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2017-09-04 14:22 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: linux-fsdevel
On Mon, Sep 04, 2017 at 11:11:04PM +0900, Tetsuo Handa wrote:
> Al Viro wrote:
> > On Mon, Sep 04, 2017 at 09:12:38PM +0900, Tetsuo Handa wrote:
> > > Hello.
> > >
> > > I hit kmemcheck splat on commit 49d31c2f389acfe8 ("dentry name snapshots") using linux-next-20170901.
> > > Changing to strncpy() fixes this problem, but using strncpy() only if CONFIG_KMEMCHECK=y is better
> > > for performance?
> >
> > *shrug*
> >
> > If anything, use dentry->d_name.len + 1 instead of DNAME_INLINE_LEN there, but
> > that's really a false positive.
>
> A false positive? Isn't it because we do
>
> dentry = kmem_cache_alloc(dentry_cache, GFP_KERNEL);
> (...snipped...)
> dname = dentry->d_iname;
> (...snipped...)
> memcpy(dname, name->name, name->len);
> dname[name->len] = 0;
>
> at __d_alloc() which leaves a room for some of d_iname[] bytes uninitialized?
> So, I think either pad explicitly at __f_alloc() or use dentry->d_name.len + 1 is needed.
It's the same situation as with
struct foo {char s[10];int v;} *p, *q;
p = malloc(sizeof struct foo);
strcpy(p->s, "foo");
p->v = 69;
...
q = malloc(sizeof struct foo);
memcpy(q, p, sizeof(struct foo));
Sure, bytes after NUL are uninitialized, but that NUL is there
in both copies. FWIW, kmemcheck could simply copy the information
about source to that about destination - in the example above
we'd end up with "q->v initialized, along with the first 4 bytes of
q->s", which matches the reality. BTW, how do you handle struct
assignments?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: fs: Uninitialized memory read at take_dentry_name_snapshot
2017-09-04 14:22 ` Al Viro
@ 2017-09-04 16:56 ` Vegard Nossum
2017-09-13 10:12 ` [PATCH] dentry: Fix kmemcheck splat at take_dentry_name_snapshot() Tetsuo Handa
0 siblings, 1 reply; 6+ messages in thread
From: Vegard Nossum @ 2017-09-04 16:56 UTC (permalink / raw)
To: Al Viro; +Cc: Tetsuo Handa, linux-fsdevel
On 4 September 2017 at 16:22, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Sep 04, 2017 at 11:11:04PM +0900, Tetsuo Handa wrote:
>> at __d_alloc() which leaves a room for some of d_iname[] bytes uninitialized?
>> So, I think either pad explicitly at __f_alloc() or use dentry->d_name.len + 1 is needed.
>
> It's the same situation as with
> struct foo {char s[10];int v;} *p, *q;
> p = malloc(sizeof struct foo);
> strcpy(p->s, "foo");
> p->v = 69;
> ...
> q = malloc(sizeof struct foo);
> memcpy(q, p, sizeof(struct foo));
>
> Sure, bytes after NUL are uninitialized, but that NUL is there
> in both copies. FWIW, kmemcheck could simply copy the information
> about source to that about destination - in the example above
> we'd end up with "q->v initialized, along with the first 4 bytes of
> q->s", which matches the reality. BTW, how do you handle struct
> assignments?
kmemcheck knows how to memcpy() the shadow memory state between two
slab-allocated objects, but it doesn't track memory state for the
stack so if you're copying partially uninitialised object to the stack
(which I'm guessing is the case here?) then it will produce the
warning Tetsuo Handa saw.
BTW as soon as msan/kmsan support for the kernel [1] is merged I am
planning to nuke kmemcheck from the kernel. msan/kmsan should handle
it properly.
[1]: https://github.com/google/kmsan
Vegard
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] dentry: Fix kmemcheck splat at take_dentry_name_snapshot()
2017-09-04 16:56 ` Vegard Nossum
@ 2017-09-13 10:12 ` Tetsuo Handa
0 siblings, 0 replies; 6+ messages in thread
From: Tetsuo Handa @ 2017-09-13 10:12 UTC (permalink / raw)
To: vegard.nossum, viro, akpm; +Cc: linux-fsdevel
Vegard Nossum wrote:
> kmemcheck knows how to memcpy() the shadow memory state between two
> slab-allocated objects, but it doesn't track memory state for the
> stack so if you're copying partially uninitialised object to the stack
> (which I'm guessing is the case here?) then it will produce the
> warning Tetsuo Handa saw.
>
> BTW as soon as msan/kmsan support for the kernel [1] is merged I am
> planning to nuke kmemcheck from the kernel. msan/kmsan should handle
> it properly.
>
> [1]: https://github.com/google/kmsan
I think kmemcheck is correct and msan/kmsan must as well splat here,
for they won't be able to know that this
unsigned char d_iname[DNAME_INLINE_LEN]; /* small names */
field is interpreted as "only bytes up to first '\0' are valid".
----------
>>From c4fe364445f7b2490209aba90b289f0543b3cfa8 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date: Wed, 13 Sep 2017 18:47:50 +0900
Subject: [PATCH] dentry: Fix kmemcheck splat at take_dentry_name_snapshot()
Since only dentry->d_name.len + 1 bytes out of DNAME_INLINE_LEN bytes are
initialized at __d_alloc(), we can't copy the whole size unconditionally.
WARNING: kmemcheck: Caught 32-bit read from uninitialized memory (ffff8fa27465ac50)
636f6e66696766732e746d70000000000010000000000000020000000188ffff
i i i i i i i i i i i i i u u u u u u u u u u i i i i i u u u u
^
RIP: 0010:take_dentry_name_snapshot+0x28/0x50
RSP: 0018:ffffa83000f5bdf8 EFLAGS: 00010246
RAX: 0000000000000020 RBX: ffff8fa274b20550 RCX: 0000000000000002
RDX: ffffa83000f5be40 RSI: ffff8fa27465ac50 RDI: ffffa83000f5be60
RBP: ffffa83000f5bdf8 R08: ffffa83000f5be48 R09: 0000000000000001
R10: ffff8fa27465ac00 R11: ffff8fa27465acc0 R12: ffff8fa27465ac00
R13: ffff8fa27465acc0 R14: 0000000000000000 R15: 0000000000000000
FS: 00007f79737ac8c0(0000) GS:ffffffff8fc30000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffff8fa274c0b000 CR3: 0000000134aa7002 CR4: 00000000000606f0
take_dentry_name_snapshot+0x28/0x50
vfs_rename+0x128/0x870
SyS_rename+0x3b2/0x3d0
entry_SYSCALL_64_fastpath+0x1a/0xa4
0xffffffffffffffff
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
| 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--git a/fs/dcache.c b/fs/dcache.c
index f901413..ad6d328 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -291,7 +291,8 @@ void take_dentry_name_snapshot(struct name_snapshot *name, struct dentry *dentry
spin_unlock(&dentry->d_lock);
name->name = p->name;
} else {
- memcpy(name->inline_name, dentry->d_iname, DNAME_INLINE_LEN);
+ memcpy(name->inline_name, dentry->d_iname,
+ dentry->d_name.len + 1);
spin_unlock(&dentry->d_lock);
name->name = name->inline_name;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-13 10:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-04 12:12 fs: Uninitialized memory read at take_dentry_name_snapshot Tetsuo Handa
2017-09-04 13:21 ` Al Viro
2017-09-04 14:11 ` Tetsuo Handa
2017-09-04 14:22 ` Al Viro
2017-09-04 16:56 ` Vegard Nossum
2017-09-13 10:12 ` [PATCH] dentry: Fix kmemcheck splat at take_dentry_name_snapshot() Tetsuo Handa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).