Re: [PATCH] fs:Fix kmemleak leak warning in getname_flags about working on unitialized memory

Re: [PATCH] fs:Fix kmemleak leak warning in getname_flags about working on unitialized memory

[Alexnader Kuleshov]

On 08-03-16, Nicholas Krause wrote:
> This fixes a kmemleak leak warning complaining about working on
> unitializied memory as found in the function, getname_flages. Seems

s/getname_flages/getname_flags

> that we are indeed working on unitialized memory, as the filename
> char pointer is never made to point to the filname structure's result
> member for holding it's name, fix this by using memcpy to copy the
> filname structure pointer's, name to the char pointer passed to this
> function.
> 
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  fs/namei.c         | 1 +
>  mm/early_ioremap.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index c386a32..6b18d57 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -196,6 +196,7 @@ getname_flags(const char __user *filename, int flags, int *empty)
>  		}
>  	}
>  
> +	memcpy((char *)result->name, filename, len);
>  	result->uptr = filename;
>  	result->aname = NULL;
>  	audit_getname(result);
> diff --git a/mm/early_ioremap.c b/mm/early_ioremap.c
> index 6d5717b..92c5235 100644
> --- a/mm/early_ioremap.c
> +++ b/mm/early_ioremap.c
> @@ -215,6 +215,7 @@ early_ioremap(resource_size_t phys_addr, unsigned long size)
>  void __init *
>  early_memremap(resource_size_t phys_addr, unsigned long size)
>  {
> +	dump_stack();

Why?

>  	return (__force void *)__early_ioremap(phys_addr, size,
>  					       FIXMAP_PAGE_NORMAL);
>  }
> -- 
> 2.7.4
> 

Re: [PATCH] fs:Fix kmemleak leak warning in getname_flags about working on unitialized memory

[Tetsuo Handa]

On 2016/08/04 6:48, Nicholas Krause wrote:
> This fixes a kmemleak leak warning complaining about working on
> unitializied memory as found in the function, getname_flages. Seems
> that we are indeed working on unitialized memory, as the filename
> char pointer is never made to point to the filname structure's result
> member for holding it's name, fix this by using memcpy to copy the
> filname structure pointer's, name to the char pointer passed to this
> function.
> 
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  fs/namei.c         | 1 +
>  mm/early_ioremap.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index c386a32..6b18d57 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -196,6 +196,7 @@ getname_flags(const char __user *filename, int flags, int *empty)
>  		}
>  	}
>  
> +	memcpy((char *)result->name, filename, len);

This filename is a __user pointer. Reading with memcpy() is not safe.

>  	result->uptr = filename;
>  	result->aname = NULL;
>  	audit_getname(result);

Re: [PATCH] fs:Fix kmemleak leak warning in getname_flags about working on unitialized memory

[Al Viro]

On Thu, Aug 04, 2016 at 09:18:19PM +0900, Tetsuo Handa wrote:
> On 2016/08/04 6:48, Nicholas Krause wrote:
> > This fixes a kmemleak leak warning complaining about working on
> > unitializied memory as found in the function, getname_flages. Seems
> > that we are indeed working on unitialized memory, as the filename
> > char pointer is never made to point to the filname structure's result
> > member for holding it's name, fix this by using memcpy to copy the
> > filname structure pointer's, name to the char pointer passed to this
> > function.
> > 
> > Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> > ---
> >  fs/namei.c         | 1 +
> >  mm/early_ioremap.c | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index c386a32..6b18d57 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -196,6 +196,7 @@ getname_flags(const char __user *filename, int flags, int *empty)
> >  		}
> >  	}
> >  
> > +	memcpy((char *)result->name, filename, len);
> 
> This filename is a __user pointer. Reading with memcpy() is not safe.

Don't feed the troll.  On all paths leading to that place we have
        result->name = kname;
        len = strncpy_from_user(kname, filename, EMBEDDED_NAME_MAX);
or
                result->name = kname;
                len = strncpy_from_user(kname, filename, PATH_MAX);
with failure exits taken if strncpy_from_user() returns an error, which means
that the damn thing has already been copied into.

FWIW, it looks a lot like buggered kmemcheck; as usual, he can't be bothered
to mention which kernel version would it be (let alone how to reproduce it
on the kernel in question), but IIRC davej had run into some instrumentation
breakage lately.

Again, don't feed the troll - you are only inviting an "improved" version
of that garbage, just as pointless.

Re: [PATCH] fs:Fix kmemleak leak warning in getname_flags about working on unitialized memory

[Eric Sandeen]

On 8/4/16 8:57 AM, Al Viro wrote:

> Don't feed the troll.  On all paths leading to that place we have
>         result->name = kname;
>         len = strncpy_from_user(kname, filename, EMBEDDED_NAME_MAX);
> or
>                 result->name = kname;
>                 len = strncpy_from_user(kname, filename, PATH_MAX);
> with failure exits taken if strncpy_from_user() returns an error, which means
> that the damn thing has already been copied into.
> 
> FWIW, it looks a lot like buggered kmemcheck; as usual, he can't be bothered
> to mention which kernel version would it be (let alone how to reproduce it
> on the kernel in question), but IIRC davej had run into some instrumentation
> breakage lately.

The original report is in https://bugzilla.kernel.org/show_bug.cgi?id=120651
if anyone is interested in it.

-Eric

> Again, don't feed the troll - you are only inviting an "improved" version
> of that garbage, just as pointless.

Re: [PATCH] fs:Fix kmemleak leak warning in getname_flags about working on unitialized memory

[Al Viro]

On Thu, Sep 01, 2016 at 08:10:44AM -0500, Eric Sandeen wrote:
> On 8/4/16 8:57 AM, Al Viro wrote:
> 
> > Don't feed the troll.  On all paths leading to that place we have
> >         result->name = kname;
> >         len = strncpy_from_user(kname, filename, EMBEDDED_NAME_MAX);
> > or
> >                 result->name = kname;
> >                 len = strncpy_from_user(kname, filename, PATH_MAX);
> > with failure exits taken if strncpy_from_user() returns an error, which means
> > that the damn thing has already been copied into.
> > 
> > FWIW, it looks a lot like buggered kmemcheck; as usual, he can't be bothered
> > to mention which kernel version would it be (let alone how to reproduce it
> > on the kernel in question), but IIRC davej had run into some instrumentation
> > breakage lately.
> 
> The original report is in https://bugzilla.kernel.org/show_bug.cgi?id=120651
> if anyone is interested in it.

	What the hell does that one have to getname_flags(), other than having
attracted the same... something on the edge of failing the Turing Test?

	FWIW, looking at the netfilter one...  That's nf_register_net_hook()
hitting
        entry->ops      = *reg;
with reg pointing to something uninitialized (according to kmemcheck, that is,
and presuming that it's not an instrumentation bug).  With the callchain
in report, it came (all in the same assumptions) from
	nf_register_net_hooks(net, ops, hweight32(table->valid_hooks))
with hweight32(table->valid_hooks) being greater than the amount of
initialized entries in ops[] (call site in ipt_register_table()).

	This "ops" ought to be net/ipv4/netfilter/iptable_filter.c:filter_ops,
allocated by
        filter_ops = xt_hook_ops_alloc(&packet_filter, iptable_filter_hook);
in iptable_filter_init().  "table" is &packet_filter and its contents ought
to be unchanged, so ->valid_hooks in there is FILTER_VALID_HOOKS, i.e.
((1 << NF_INET_LOCAL_IN) | (1 << NF_INET_FORWARD) | (1 << NF_INET_LOCAL_OUT)).

	Which is to say, filter_ops[] had fewer than 3 initialized elements
when it got to the call of iptable_filter_table_init()...  Since filter_ops
hadn't been NULL, the xt_hook_ops_alloc() call above must've already been
done.  Said xt_hook_ops_alloc() should've allocated a 3-element array and
hooked through all of it, so it's not a wholesale uninitialized element, it's
uninitialized parts of one...

	What gets initialized is ->hook, ->pf, ->hooknum and ->priority.
Let's figure out the offsets:
	0: list (two pointers, i.e. 16 bytes)
	0x10: hook (8)
	0x18: dev (8)
	0x20: priv (8)
	0x28: pf (1)
	0x29: padding (3)
	0x2c: hooknum (4)
	0x30: priority (4)
	0x34: padding (8)
	
OK...  The address of the damn thing is apparently ffff880037b4bd80 and
we see complaint about the accesses at offsets 0, 0x18, 8, 0x20 and then
the same pattern with 0x38 and 0x70 added (i.e. the same fields in the next
two elements of the same array).  Then there are similar complaints, but
with a different call chain (iptable_mangle instead of iptable_filter).

These offsets are ->list, ->dev and ->priv, and those are exactly the ones
not initialized by xt_hook_ops_alloc().  Looking at the nf_register_net_hook(),
we have
        list_add_rcu(&entry->ops.list, elem->list.prev);
a bit further down the road.  ->dev and ->priv are left uninitialized (and
very likely - unused).

I would say it's a false positive.  struct nf_hook_ops is embedded into a
bunch of different objects, with different subsets of fields getting used.
IMO it's a bad idea (in particular, I really wonder if ->list would've
been better off moved into (some of) the containing suckers), but it's
not a bug per se, just a design choice asking for trouble.  One way of
getting kmemcheck off your back would be to switch xt_hook_ops_alloc() from
	ops = kmalloc(sizeof(*ops) * num_hooks, GFP_KERNEL);
to
	ops = kcalloc(num_hooks, sizeof(*ops), GFP_KERNEL);
which might have some merits beyond making kmemcheck STFU...

Re: [PATCH] fs:Fix kmemleak leak warning in getname_flags about working on unitialized memory

[Eric Sandeen]

On 9/1/16 12:16 PM, Al Viro wrote:
> On Thu, Sep 01, 2016 at 08:10:44AM -0500, Eric Sandeen wrote:
>> On 8/4/16 8:57 AM, Al Viro wrote:
>>
>>> Don't feed the troll.  On all paths leading to that place we have
>>>         result->name = kname;
>>>         len = strncpy_from_user(kname, filename, EMBEDDED_NAME_MAX);
>>> or
>>>                 result->name = kname;
>>>                 len = strncpy_from_user(kname, filename, PATH_MAX);
>>> with failure exits taken if strncpy_from_user() returns an error, which means
>>> that the damn thing has already been copied into.
>>>
>>> FWIW, it looks a lot like buggered kmemcheck; as usual, he can't be bothered
>>> to mention which kernel version would it be (let alone how to reproduce it
>>> on the kernel in question), but IIRC davej had run into some instrumentation
>>> breakage lately.
>>
>> The original report is in https://bugzilla.kernel.org/show_bug.cgi?id=120651
>> if anyone is interested in it.
> 
> 	What the hell does that one have to getname_flags(), other than having
> attracted the same... something on the edge of failing the Turing Test?

Sigh, pasted the wrong one, thus making things worse.

I suppose reverse-engineering Nick is a fool's errand, only adding to the
noise, sorry.  I'll stop.  ;)

-Eric

Re: [PATCH] fs:Fix kmemleak leak warning in getname_flags about working on unitialized memory

[Vlastimil Babka]

On 08/03/2016 11:48 PM, Nicholas Krause wrote:
> This fixes a kmemleak leak warning complaining about working on
> unitializied memory as found in the function, getname_flages. Seems

What exactly is the kmemleak warning saying?

> that we are indeed working on unitialized memory, as the filename
> char pointer is never made to point to the filname structure's result
> member for holding it's name, fix this by using memcpy to copy the
> filname structure pointer's, name to the char pointer passed to this
> function.

I don't understand what you're saying here. "the char pointer passed to
this function" is the source, not destination.

> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  fs/namei.c         | 1 +
>  mm/early_ioremap.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index c386a32..6b18d57 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -196,6 +196,7 @@ getname_flags(const char __user *filename, int flags, int *empty)
>  		}
>  	}
>  
> +	memcpy((char *)result->name, filename, len);

This will be wrong even with strncpy_from_user instead of memcpy. AFAICS
result->name already points to a copy of filename.
Also if you think that the above is "copy[ing] the filname structure
pointer's, name to the char pointer passed to this function" then you
are wrong.

>  	result->uptr = filename;
>  	result->aname = NULL;
>  	audit_getname(result);
> diff --git a/mm/early_ioremap.c b/mm/early_ioremap.c
> index 6d5717b..92c5235 100644
> --- a/mm/early_ioremap.c
> +++ b/mm/early_ioremap.c
> @@ -215,6 +215,7 @@ early_ioremap(resource_size_t phys_addr, unsigned long size)
>  void __init *
>  early_memremap(resource_size_t phys_addr, unsigned long size)
>  {
> +	dump_stack();
>  	return (__force void *)__early_ioremap(phys_addr, size,
>  					       FIXMAP_PAGE_NORMAL);
>  }
>