[PATCH] nf_conntrack: avoid kernel pointer value leak in slab name

[PATCH] nf_conntrack: avoid kernel pointer value leak in slab name

[Linus Torvalds]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sat, 14 May 2016 11:11:44 -0700
Subject: [PATCH] nf_conntrack: avoid kernel pointer value leak in slab name

The slab name ends up being visible in the directory structure under
/sys, and even if you don't have access rights to the file you can see
the filenames.

Just use a 64-bit counter instead of the pointer to the 'net' structure
to generate a unique name.

This code will go away in 4.7 when the conntrack code moves to a single
kmemcache, but this is the backportable simple solution to avoiding
leaking kernel pointers to user space.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: stable@vger.kernel.org
---

This would seem to be the minimal patch.

Eric - I marked you as "acking" this patch from the discussion. It's not 
actually any of the exact patches that were flying around, but close 
enough..

It's been "tested" by booting and looking at the end result. Seems to 
work, and it's not exactly complicated.

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 895d11dced3c..e27fd17c6743 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1778,6 +1778,7 @@ void nf_conntrack_init_end(void)
 
 int nf_conntrack_init_net(struct net *net)
 {
+	static atomic64_t unique_id;
 	int ret = -ENOMEM;
 	int cpu;
 
@@ -1800,7 +1801,8 @@ int nf_conntrack_init_net(struct net *net)
 	if (!net->ct.stat)
 		goto err_pcpu_lists;
 
-	net->ct.slabname = kasprintf(GFP_KERNEL, "nf_conntrack_%p", net);
+	net->ct.slabname = kasprintf(GFP_KERNEL, "nf_conntrack_%llu",
+				(u64)atomic64_inc_return(&unique_id));
 	if (!net->ct.slabname)
 		goto err_slabname;
 

Re: [PATCH] nf_conntrack: avoid kernel pointer value leak in slab name

[Eric Dumazet]

On Sat, 2016-05-14 at 11:24 -0700, Linus Torvalds wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Sat, 14 May 2016 11:11:44 -0700
> Subject: [PATCH] nf_conntrack: avoid kernel pointer value leak in slab name
> 
> The slab name ends up being visible in the directory structure under
> /sys, and even if you don't have access rights to the file you can see
> the filenames.
> 
> Just use a 64-bit counter instead of the pointer to the 'net' structure
> to generate a unique name.
> 
> This code will go away in 4.7 when the conntrack code moves to a single
> kmemcache, but this is the backportable simple solution to avoiding
> leaking kernel pointers to user space.
> 
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: stable@vger.kernel.org
> ---
> 
> This would seem to be the minimal patch.
> 
> Eric - I marked you as "acking" this patch from the discussion. It's not 
> actually any of the exact patches that were flying around, but close 
> enough..
> 
> It's been "tested" by booting and looking at the end result. Seems to 
> work, and it's not exactly complicated.
> 
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index 895d11dced3c..e27fd17c6743 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -1778,6 +1778,7 @@ void nf_conntrack_init_end(void)
>  
>  int nf_conntrack_init_net(struct net *net)
>  {
> +	static atomic64_t unique_id;
>  	int ret = -ENOMEM;
>  	int cpu;
>  
> @@ -1800,7 +1801,8 @@ int nf_conntrack_init_net(struct net *net)
>  	if (!net->ct.stat)
>  		goto err_pcpu_lists;
>  
> -	net->ct.slabname = kasprintf(GFP_KERNEL, "nf_conntrack_%p", net);
> +	net->ct.slabname = kasprintf(GFP_KERNEL, "nf_conntrack_%llu",
> +				(u64)atomic64_inc_return(&unique_id));
>  	if (!net->ct.slabname)
>  		goto err_slabname;
>  

SGTM, thanks Linus.

Fixes: 5b3501faa874 ("netfilter: nf_conntrack: per netns nf_conntrack_cachep")

Re: [PATCH] nf_conntrack: avoid kernel pointer value leak in slab name

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sat, 14 May 2016 11:24:08 -0700 (PDT)

> 
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Sat, 14 May 2016 11:11:44 -0700
> Subject: [PATCH] nf_conntrack: avoid kernel pointer value leak in slab name
> 
> The slab name ends up being visible in the directory structure under
> /sys, and even if you don't have access rights to the file you can see
> the filenames.
> 
> Just use a 64-bit counter instead of the pointer to the 'net' structure
> to generate a unique name.
> 
> This code will go away in 4.7 when the conntrack code moves to a single
> kmemcache, but this is the backportable simple solution to avoiding
> leaking kernel pointers to user space.
> 
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: stable@vger.kernel.org

Applied, thanks.

Re: [PATCH] nf_conntrack: avoid kernel pointer value leak in slab name

[Linus Torvalds]

On Sat, May 14, 2016 at 11:24 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
>
> -       net->ct.slabname = kasprintf(GFP_KERNEL, "nf_conntrack_%p", net);
> +       net->ct.slabname = kasprintf(GFP_KERNEL, "nf_conntrack_%llu",
> +                               (u64)atomic64_inc_return(&unique_id));

Oh well. I suspect this is going to cause a new warning on alpha and
ia64 and possibly others.

"u64" is indeed "unsigned long long" on x86 and many other
architectures, but on alpga and ia64 it's just "unsigned long".

So that case should have been to "long long". I detest how there isn't
a "64-bit size" printf specifier.

And no, PRId64 preprocessor garbage and similar disgusting hacks by
POSIX isn't the answer. I think MSC actually got that right with
"%I64d".

Oh well. It's a fairly harmless compiler warning, it isn't a code
correctness issue.

               Linus

Re: [PATCH] nf_conntrack: avoid kernel pointer value leak in slab name

[Willy Tarreau]

On Sat, May 14, 2016 at 02:31:04PM -0700, Linus Torvalds wrote:
> On Sat, May 14, 2016 at 11:24 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> >
> > -       net->ct.slabname = kasprintf(GFP_KERNEL, "nf_conntrack_%p", net);
> > +       net->ct.slabname = kasprintf(GFP_KERNEL, "nf_conntrack_%llu",
> > +                               (u64)atomic64_inc_return(&unique_id));
> 
> Oh well. I suspect this is going to cause a new warning on alpha and
> ia64 and possibly others.
> 
> "u64" is indeed "unsigned long long" on x86 and many other
> architectures, but on alpga and ia64 it's just "unsigned long".
> 
> So that case should have been to "long long". I detest how there isn't
> a "64-bit size" printf specifier.

Why simply not cast the atomic to (unsigned long long) instead of (u64)
so that %llu always matches ?

Willy

Re: [PATCH] nf_conntrack: avoid kernel pointer value leak in slab name

[Linus Torvalds]

On Sat, May 14, 2016 at 2:33 PM, Willy Tarreau <w@1wt.eu> wrote:
>
> Why simply not cast the atomic to (unsigned long long) instead of (u64)
> so that %llu always matches ?

Yes, that fixes the problem. It's just more typing, and annoying. The
fact that MS got it right while posix and gcc screwed it up is a bit
embarrassing..

If we ever start using __uint128_t, we'll have even more problems in
this area. Oh well.

                Linus

Re: [PATCH] nf_conntrack: avoid kernel pointer value leak in slab name

[Willy Tarreau]

On Sat, May 14, 2016 at 03:21:31PM -0700, Linus Torvalds wrote:
> On Sat, May 14, 2016 at 2:33 PM, Willy Tarreau <w@1wt.eu> wrote:
> >
> > Why simply not cast the atomic to (unsigned long long) instead of (u64)
> > so that %llu always matches ?
> 
> Yes, that fixes the problem. It's just more typing, and annoying. The
> fact that MS got it right while posix and gcc screwed it up is a bit
> embarrassing..

Well on the other hand, because of this MS still has problems porting
code from 32 to 64 bit. The real problem is that on both sides they
imagined that you needed only one way to specify your types. In practice
users generally want either the most optimal types for the architecture
because they don't care about the size (char, int, size_t, void *...)
or a specific size. This last one is annoying to use with printf format.

> If we ever start using __uint128_t, we'll have even more problems in
> this area. Oh well.

Definitely.

Willy

Re: [PATCH] nf_conntrack: avoid kernel pointer value leak in slab name

[Linus Torvalds]

On Sat, May 14, 2016 at 2:31 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> "u64" is indeed "unsigned long long" on x86 and many other
> architectures, but on alpha and ia64 it's just "unsigned long".

Actually, I take that back.

In the kernel, it seems to always be "unsigned long long", even on
alpha and ia64.

We do have a "int-l64.h" file that typedef's __u64 to be just unsigned
long, and yes, that file is included for alpha and ia64, but it seems
that that only happens when __KERNEL__ is not defined.

So it does seem like using "%ull" and u64 is fine. Not in general, but
inside the kernel it's ok.

              Linus