LinuxLists.cc - NULL pointer dereference in selinux_ip_postroute

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Tue, Aug 7, 2012 at 2:12 PM, John Stultz <[email protected]> wrote:
> Hi,
> With my kvm environment using 3.6-rc1+, I'm seeing NULL pointer
> dereferences in selinux_ip_postroute_compat(). It looks like the sksec value
> is null and we die in the following line:
>
> if (selinux_xfrm_postroute_last(sksec->sid, skb, &ad, proto))
>
> This triggers every time I shutdown the machine, but has also triggered
> randomly after a few hours.
>
> This is on an ubuntu 12.04 image, not using selinux.

NOTE: Adding the SELinux list to the CC line

Hi,

I'm trying to understand this and I was hoping you could you clarify a
few things for me:

* Is the panic in the Ubuntu 12.04 guest, or the host? If the host,
could you share what distribution you are using?
* When you say you are not using SELinux, could you be more specific?
It seems odd that you are not using SELinux but the panic is happening
in a SELinux hook.

Thanks.

> Running with the following kvm line:
> kvm -nographic -smp 4 -m 1G -hda disk.img -net user -net nic,model=virtio
> -redir tcp:4400::22 -kernel ./bzImage -initrd initrd.img-1-jstultz -append
> "root=UUID=b08aa86a-4b16-488f-a3de-33c2cf335bf0 ro console=ttyS0,115200n8"
>
> Two different traces below. Config attached.

--
paul moore
http://www.paul-moore.com

2012-08-07 21:58:45

by John Stultz

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On 08/07/2012 02:50 PM, Paul Moore wrote:
> On Tue, Aug 7, 2012 at 2:12 PM, John Stultz <[email protected]> wrote:
>> Hi,
>> With my kvm environment using 3.6-rc1+, I'm seeing NULL pointer
>> dereferences in selinux_ip_postroute_compat(). It looks like the sksec value
>> is null and we die in the following line:
>>
>> if (selinux_xfrm_postroute_last(sksec->sid, skb, &ad, proto))
>>
>> This triggers every time I shutdown the machine, but has also triggered
>> randomly after a few hours.
>>
>> This is on an ubuntu 12.04 image, not using selinux.
> NOTE: Adding the SELinux list to the CC line
Thanks!

>
> Hi,
>
> I'm trying to understand this and I was hoping you could you clarify a
> few things for me:
>
> * Is the panic in the Ubuntu 12.04 guest, or the host? If the host,
> could you share what distribution you are using?
Sorry, its a 12.04 guest. I think the host is Ubuntu 12.04 as well.

> * When you say you are not using SELinux, could you be more specific?
> It seems odd that you are not using SELinux but the panic is happening
> in a SELinux hook.
I just mean that, being Ubuntu, the system (userland) isn't configured
to use selinux. SELinux is just enabled in the kernel config.

thanks
-john

2012-08-07 22:01:40

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Tue, Aug 7, 2012 at 5:58 PM, John Stultz <[email protected]> wrote:
> On 08/07/2012 02:50 PM, Paul Moore wrote:
>>
>> On Tue, Aug 7, 2012 at 2:12 PM, John Stultz <[email protected]>
>> wrote:
>>>
>>> Hi,
>>> With my kvm environment using 3.6-rc1+, I'm seeing NULL pointer
>>> dereferences in selinux_ip_postroute_compat(). It looks like the sksec
>>> value
>>> is null and we die in the following line:
>>>
>>> if (selinux_xfrm_postroute_last(sksec->sid, skb, &ad, proto))
>>>
>>> This triggers every time I shutdown the machine, but has also triggered
>>> randomly after a few hours.
>>>
>>> This is on an ubuntu 12.04 image, not using selinux.
>>
>> NOTE: Adding the SELinux list to the CC line
>
> Thanks!
>
>> Hi,
>>
>> I'm trying to understand this and I was hoping you could you clarify a
>> few things for me:
>>
>> * Is the panic in the Ubuntu 12.04 guest, or the host? If the host,
>> could you share what distribution you are using?
>
> Sorry, its a 12.04 guest. I think the host is Ubuntu 12.04 as well.
>
>
>> * When you say you are not using SELinux, could you be more specific?
>> It seems odd that you are not using SELinux but the panic is happening
>> in a SELinux hook.
>
> I just mean that, being Ubuntu, the system (userland) isn't configured to
> use selinux. SELinux is just enabled in the kernel config.

Thanks for the quick response, I'll setup an Ubuntu guest and see if I
can reproduce this ... something is odd. Anything non-standard about
your guest install or anything else you think might be helpful?

--
paul moore
http://www.paul-moore.com

2012-08-07 22:16:21

by Serge E. Hallyn

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

Quoting Paul Moore ([email protected]):
> On Tue, Aug 7, 2012 at 5:58 PM, John Stultz <[email protected]> wrote:
> > On 08/07/2012 02:50 PM, Paul Moore wrote:
> >>
> >> On Tue, Aug 7, 2012 at 2:12 PM, John Stultz <[email protected]>
> >> wrote:
> >>>
> >>> Hi,
> >>> With my kvm environment using 3.6-rc1+, I'm seeing NULL pointer
> >>> dereferences in selinux_ip_postroute_compat(). It looks like the sksec
> >>> value
> >>> is null and we die in the following line:
> >>>
> >>> if (selinux_xfrm_postroute_last(sksec->sid, skb, &ad, proto))
> >>>
> >>> This triggers every time I shutdown the machine, but has also triggered
> >>> randomly after a few hours.
> >>>
> >>> This is on an ubuntu 12.04 image, not using selinux.
> >>
> >> NOTE: Adding the SELinux list to the CC line
> >
> > Thanks!
> >
> >> Hi,
> >>
> >> I'm trying to understand this and I was hoping you could you clarify a
> >> few things for me:
> >>
> >> * Is the panic in the Ubuntu 12.04 guest, or the host? If the host,
> >> could you share what distribution you are using?
> >
> > Sorry, its a 12.04 guest. I think the host is Ubuntu 12.04 as well.
> >
> >
> >> * When you say you are not using SELinux, could you be more specific?
> >> It seems odd that you are not using SELinux but the panic is happening
> >> in a SELinux hook.
> >
> > I just mean that, being Ubuntu, the system (userland) isn't configured to
> > use selinux. SELinux is just enabled in the kernel config.
>
> Thanks for the quick response, I'll setup an Ubuntu guest and see if I
> can reproduce this ... something is odd. Anything non-standard about
> your guest install or anything else you think might be helpful?

The problem seems to be that selinux_nf_ip_init() was called, which
registers the selinux_ipv4_ops (and ipv6). Those should not get registered
if selinux ends up not being loaded (as in, if apparmor is loaded first),
since as you've found here the selinux lsm hooks won't be called to set
call selinux_sk_alloc_security().

I assume what's happening is that CONFIG_SECURITY_SELINUX_BOOTPARAM_VALUE was
set to 1, but selinux ended up being set to disabled after the
__initcall(selinux_nf_ip_init) ran? Weird.

-serge

2012-08-07 22:24:01

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Tuesday, August 07, 2012 10:17:32 PM Serge E. Hallyn wrote:
> Quoting Paul Moore ([email protected]):
> > On Tue, Aug 7, 2012 at 5:58 PM, John Stultz <[email protected]>
wrote:
> > > On 08/07/2012 02:50 PM, Paul Moore wrote:
> > >> On Tue, Aug 7, 2012 at 2:12 PM, John Stultz <[email protected]>
> > >>
> > >> wrote:
> > >>> Hi,
> > >>>
> > >>> With my kvm environment using 3.6-rc1+, I'm seeing NULL pointer
> > >>>
> > >>> dereferences in selinux_ip_postroute_compat(). It looks like the sksec
> > >>> value
> > >>>
> > >>> is null and we die in the following line:
> > >>> if (selinux_xfrm_postroute_last(sksec->sid, skb, &ad, proto))
> > >>>
> > >>> This triggers every time I shutdown the machine, but has also
> > >>> triggered
> > >>> randomly after a few hours.
> > >>>
> > >>> This is on an ubuntu 12.04 image, not using selinux.
> > >>
> > >> NOTE: Adding the SELinux list to the CC line
> > >
> > > Thanks!
> > >
> > >> Hi,
> > >>
> > >> I'm trying to understand this and I was hoping you could you clarify a
> > >> few things for me:
> > >>
> > >> * Is the panic in the Ubuntu 12.04 guest, or the host? If the host,
> > >> could you share what distribution you are using?
> > >
> > > Sorry, its a 12.04 guest. I think the host is Ubuntu 12.04 as well.
> > >
> > >> * When you say you are not using SELinux, could you be more specific?
> > >> It seems odd that you are not using SELinux but the panic is happening
> > >> in a SELinux hook.
> > >
> > > I just mean that, being Ubuntu, the system (userland) isn't configured
> > > to
> > > use selinux. SELinux is just enabled in the kernel config.
> >
> > Thanks for the quick response, I'll setup an Ubuntu guest and see if I
> > can reproduce this ... something is odd. Anything non-standard about
> > your guest install or anything else you think might be helpful?
>
> The problem seems to be that selinux_nf_ip_init() was called, which
> registers the selinux_ipv4_ops (and ipv6). Those should not get registered
> if selinux ends up not being loaded (as in, if apparmor is loaded first),
> since as you've found here the selinux lsm hooks won't be called to set
> call selinux_sk_alloc_security().
>
> I assume what's happening is that CONFIG_SECURITY_SELINUX_BOOTPARAM_VALUE
> was set to 1, but selinux ended up being set to disabled after the
> __initcall(selinux_nf_ip_init) ran? Weird.

Yeah, nothing obvious is jumping out at me in the code except for some weird
race condition like you mention above. I'm downloading an Ubuntu ISO right
now, it should be ready to play with tomorrow morning.

--
paul moore
http://www.paul-moore.com

2012-08-07 22:27:04

by John Stultz

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On 08/07/2012 03:01 PM, Paul Moore wrote:
> On Tue, Aug 7, 2012 at 5:58 PM, John Stultz <[email protected]> wrote:
>> On 08/07/2012 02:50 PM, Paul Moore wrote:
>>> On Tue, Aug 7, 2012 at 2:12 PM, John Stultz <[email protected]>
>>> wrote:
>>>> Hi,
>>>> With my kvm environment using 3.6-rc1+, I'm seeing NULL pointer
>>>> dereferences in selinux_ip_postroute_compat(). It looks like the sksec
>>>> value
>>>> is null and we die in the following line:
>>>>
>>>> if (selinux_xfrm_postroute_last(sksec->sid, skb, &ad, proto))
>>>>
>>>> This triggers every time I shutdown the machine, but has also triggered
>>>> randomly after a few hours.
>>>>
>>>> This is on an ubuntu 12.04 image, not using selinux.
>>> NOTE: Adding the SELinux list to the CC line
>> Thanks!
>>
>>> Hi,
>>>
>>> I'm trying to understand this and I was hoping you could you clarify a
>>> few things for me:
>>>
>>> * Is the panic in the Ubuntu 12.04 guest, or the host? If the host,
>>> could you share what distribution you are using?
>> Sorry, its a 12.04 guest. I think the host is Ubuntu 12.04 as well.
>>
>>
>>> * When you say you are not using SELinux, could you be more specific?
>>> It seems odd that you are not using SELinux but the panic is happening
>>> in a SELinux hook.
>> I just mean that, being Ubuntu, the system (userland) isn't configured to
>> use selinux. SELinux is just enabled in the kernel config.
> Thanks for the quick response, I'll setup an Ubuntu guest and see if I
> can reproduce this ... something is odd. Anything non-standard about
> your guest install or anything else you think might be helpful?
Don't think so. Just a standard 64bit ubuntu 12.04 install.

Since I'm booting kernel/initrd from the commandline, the initrd *may*
be older then 12.04, I can't quite remember when I copied that out of
the image. I'll see if it still triggers if I copy the current initrd out.

thanks
-john

2012-08-07 23:05:35

by John Stultz

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On 08/07/2012 03:17 PM, Serge E. Hallyn wrote:
> Quoting Paul Moore ([email protected]):
>> On Tue, Aug 7, 2012 at 5:58 PM, John Stultz <[email protected]> wrote:
>>> On 08/07/2012 02:50 PM, Paul Moore wrote:
>>>> On Tue, Aug 7, 2012 at 2:12 PM, John Stultz <[email protected]>
>>>> wrote:
>>>>> Hi,
>>>>> With my kvm environment using 3.6-rc1+, I'm seeing NULL pointer
>>>>> dereferences in selinux_ip_postroute_compat(). It looks like the sksec
>>>>> value
>>>>> is null and we die in the following line:
>>>>>
>>>>> if (selinux_xfrm_postroute_last(sksec->sid, skb, &ad, proto))
>>>>>
>>>>> This triggers every time I shutdown the machine, but has also triggered
>>>>> randomly after a few hours.
>>>>>
>>>>> This is on an ubuntu 12.04 image, not using selinux.
>>>> NOTE: Adding the SELinux list to the CC line
>>> Thanks!
>>>
>>>> Hi,
>>>>
>>>> I'm trying to understand this and I was hoping you could you clarify a
>>>> few things for me:
>>>>
>>>> * Is the panic in the Ubuntu 12.04 guest, or the host? If the host,
>>>> could you share what distribution you are using?
>>> Sorry, its a 12.04 guest. I think the host is Ubuntu 12.04 as well.
>>>
>>>
>>>> * When you say you are not using SELinux, could you be more specific?
>>>> It seems odd that you are not using SELinux but the panic is happening
>>>> in a SELinux hook.
>>> I just mean that, being Ubuntu, the system (userland) isn't configured to
>>> use selinux. SELinux is just enabled in the kernel config.
>> Thanks for the quick response, I'll setup an Ubuntu guest and see if I
>> can reproduce this ... something is odd. Anything non-standard about
>> your guest install or anything else you think might be helpful?
> The problem seems to be that selinux_nf_ip_init() was called, which
> registers the selinux_ipv4_ops (and ipv6). Those should not get registered
> if selinux ends up not being loaded (as in, if apparmor is loaded first),
> since as you've found here the selinux lsm hooks won't be called to set
> call selinux_sk_alloc_security().
This sounds about right:
root@testvm:~# dmesg | grep SELinux
[ 0.004578] SELinux: Initializing.
[ 0.005704] SELinux: Starting in permissive mode
[ 2.235034] SELinux: Registering netfilter hooks

> I assume what's happening is that CONFIG_SECURITY_SELINUX_BOOTPARAM_VALUE was
> set to 1, but selinux ended up being set to disabled after the
> __initcall(selinux_nf_ip_init) ran? Weird.
This looks right as well:

# zcat config.gz | grep SELINUX
CONFIG_SECURITY_SELINUX=y
CONFIG_SECURITY_SELINUX_BOOTPARAM=y
CONFIG_SECURITY_SELINUX_BOOTPARAM_VALUE=1
CONFIG_SECURITY_SELINUX_DISABLE=y
CONFIG_SECURITY_SELINUX_DEVELOP=y
CONFIG_SECURITY_SELINUX_AVC_STATS=y
CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE=1
# CONFIG_SECURITY_SELINUX_POLICYDB_VERSION_MAX is not set
CONFIG_DEFAULT_SECURITY_SELINUX=y

Since the problem isn't completely obvious, I'm starting a bisection to
narrow this down some more.

thanks
-john

2012-08-07 23:24:56

by john stultz

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On 08/07/2012 03:26 PM, John Stultz wrote:
> On 08/07/2012 03:01 PM, Paul Moore wrote:
>> On Tue, Aug 7, 2012 at 5:58 PM, John Stultz <[email protected]>
>> wrote:
>>> On 08/07/2012 02:50 PM, Paul Moore wrote:
>>>> On Tue, Aug 7, 2012 at 2:12 PM, John Stultz <[email protected]>
>>>> wrote:
>>>>> Hi,
>>>>> With my kvm environment using 3.6-rc1+, I'm seeing NULL pointer
>>>>> dereferences in selinux_ip_postroute_compat(). It looks like the
>>>>> sksec
>>>>> value
>>>>> is null and we die in the following line:
>>>>>
>>>>> if (selinux_xfrm_postroute_last(sksec->sid, skb, &ad, proto))
>>>>>
>>>>> This triggers every time I shutdown the machine, but has also
>>>>> triggered
>>>>> randomly after a few hours.
>>>>>
>>>>> This is on an ubuntu 12.04 image, not using selinux.
>>>> NOTE: Adding the SELinux list to the CC line
>>> Thanks!
>>>
>>>> Hi,
>>>>
>>>> I'm trying to understand this and I was hoping you could you clarify a
>>>> few things for me:
>>>>
>>>> * Is the panic in the Ubuntu 12.04 guest, or the host? If the host,
>>>> could you share what distribution you are using?
>>> Sorry, its a 12.04 guest. I think the host is Ubuntu 12.04 as well.
>>>
>>>
>>>> * When you say you are not using SELinux, could you be more specific?
>>>> It seems odd that you are not using SELinux but the panic is happening
>>>> in a SELinux hook.
>>> I just mean that, being Ubuntu, the system (userland) isn't
>>> configured to
>>> use selinux. SELinux is just enabled in the kernel config.
>> Thanks for the quick response, I'll setup an Ubuntu guest and see if I
>> can reproduce this ... something is odd. Anything non-standard about
>> your guest install or anything else you think might be helpful?
> Don't think so. Just a standard 64bit ubuntu 12.04 install.
>
> Since I'm booting kernel/initrd from the commandline, the initrd *may*
> be older then 12.04, I can't quite remember when I copied that out of
> the image. I'll see if it still triggers if I copy the current initrd
> out.

Nope, that's not it, I just triggered the same thing w/ the Ubuntu 12.04
initrd on the image.

thanks
-john

2012-08-08 16:58:45

by John Johansen

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On 08/07/2012 03:17 PM, Serge E. Hallyn wrote:
> Quoting Paul Moore ([email protected]):
>> On Tue, Aug 7, 2012 at 5:58 PM, John Stultz <[email protected]> wrote:
>>> On 08/07/2012 02:50 PM, Paul Moore wrote:
>>>>
>>>> On Tue, Aug 7, 2012 at 2:12 PM, John Stultz <[email protected]>
>>>> wrote:
>>>>>
>>>>> Hi,
>>>>> With my kvm environment using 3.6-rc1+, I'm seeing NULL pointer
>>>>> dereferences in selinux_ip_postroute_compat(). It looks like the sksec
>>>>> value
>>>>> is null and we die in the following line:
>>>>>
>>>>> if (selinux_xfrm_postroute_last(sksec->sid, skb, &ad, proto))
>>>>>
>>>>> This triggers every time I shutdown the machine, but has also triggered
>>>>> randomly after a few hours.
>>>>>
>>>>> This is on an ubuntu 12.04 image, not using selinux.
>>>>
>>>> NOTE: Adding the SELinux list to the CC line
>>>
>>> Thanks!
>>>
>>>> Hi,
>>>>
>>>> I'm trying to understand this and I was hoping you could you clarify a
>>>> few things for me:
>>>>
>>>> * Is the panic in the Ubuntu 12.04 guest, or the host? If the host,
>>>> could you share what distribution you are using?
>>>
>>> Sorry, its a 12.04 guest. I think the host is Ubuntu 12.04 as well.
>>>
>>>
>>>> * When you say you are not using SELinux, could you be more specific?
>>>> It seems odd that you are not using SELinux but the panic is happening
>>>> in a SELinux hook.
>>>
>>> I just mean that, being Ubuntu, the system (userland) isn't configured to
>>> use selinux. SELinux is just enabled in the kernel config.
>>
>> Thanks for the quick response, I'll setup an Ubuntu guest and see if I
>> can reproduce this ... something is odd. Anything non-standard about
>> your guest install or anything else you think might be helpful?
>
> The problem seems to be that selinux_nf_ip_init() was called, which
> registers the selinux_ipv4_ops (and ipv6). Those should not get registered
> if selinux ends up not being loaded (as in, if apparmor is loaded first),
> since as you've found here the selinux lsm hooks won't be called to set
> call selinux_sk_alloc_security().
>
> I assume what's happening is that CONFIG_SECURITY_SELINUX_BOOTPARAM_VALUE was
> set to 1, but selinux ended up being set to disabled after the
> __initcall(selinux_nf_ip_init) ran? Weird.
>
Its not an Ubuntu kernel. The config has selinux set as the only LSM and
it is configured to be on by default

2012-08-08 19:15:33

by john stultz

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On 08/07/2012 03:37 PM, John Stultz wrote:
> On 08/07/2012 03:17 PM, Serge E. Hallyn wrote:
>> Quoting Paul Moore ([email protected]):
>>> On Tue, Aug 7, 2012 at 5:58 PM, John Stultz <[email protected]>
>>> wrote:
>>>> On 08/07/2012 02:50 PM, Paul Moore wrote:
>>>>> On Tue, Aug 7, 2012 at 2:12 PM, John Stultz <[email protected]>
>>>>> wrote:
>>>>>> Hi,
>>>>>> With my kvm environment using 3.6-rc1+, I'm seeing NULL
>>>>>> pointer
>>>>>> dereferences in selinux_ip_postroute_compat(). It looks like the
>>>>>> sksec
>>>>>> value
>>>>>> is null and we die in the following line:
>>>>>>
>>>>>> if (selinux_xfrm_postroute_last(sksec->sid, skb, &ad, proto))
>>>>>>
>>>>>> This triggers every time I shutdown the machine, but has also
>>>>>> triggered
>>>>>> randomly after a few hours.
[snip]
>> The problem seems to be that selinux_nf_ip_init() was called, which
>> registers the selinux_ipv4_ops (and ipv6). Those should not get
>> registered
>> if selinux ends up not being loaded (as in, if apparmor is loaded
>> first),
>> since as you've found here the selinux lsm hooks won't be called to set
>> call selinux_sk_alloc_security().
> This sounds about right:
> root@testvm:~# dmesg | grep SELinux
> [ 0.004578] SELinux: Initializing.
> [ 0.005704] SELinux: Starting in permissive mode
> [ 2.235034] SELinux: Registering netfilter hooks
>
>> I assume what's happening is that
>> CONFIG_SECURITY_SELINUX_BOOTPARAM_VALUE was
>> set to 1, but selinux ended up being set to disabled after the
>> __initcall(selinux_nf_ip_init) ran? Weird.
> This looks right as well:
>
> # zcat config.gz | grep SELINUX
> CONFIG_SECURITY_SELINUX=y
> CONFIG_SECURITY_SELINUX_BOOTPARAM=y
> CONFIG_SECURITY_SELINUX_BOOTPARAM_VALUE=1
> CONFIG_SECURITY_SELINUX_DISABLE=y
> CONFIG_SECURITY_SELINUX_DEVELOP=y
> CONFIG_SECURITY_SELINUX_AVC_STATS=y
> CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE=1
> # CONFIG_SECURITY_SELINUX_POLICYDB_VERSION_MAX is not set
> CONFIG_DEFAULT_SECURITY_SELINUX=y
>
>
> Since the problem isn't completely obvious, I'm starting a bisection
> to narrow this down some more.

So I bisected this down and it seems to be the following commit:

commit be9f4a44e7d41cee50ddb5f038fc2391cbbb4046
Author: Eric Dumazet <[email protected]>
Date: Thu Jul 19 07:34:03 2012 +0000

ipv4: tcp: remove per net tcp_sock

It doesn't revert totally cleanly, but after fixing up the rejections
and booting with this patch removed on top of Linus' head the oops on
shutdown goes away.

thanks
-john

2012-08-08 19:27:00

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Wednesday, August 08, 2012 12:14:42 PM John Stultz wrote:
> So I bisected this down and it seems to be the following commit:
>
> commit be9f4a44e7d41cee50ddb5f038fc2391cbbb4046
> Author: Eric Dumazet <[email protected]>
> Date: Thu Jul 19 07:34:03 2012 +0000
>
> ipv4: tcp: remove per net tcp_sock
>
>
> It doesn't revert totally cleanly, but after fixing up the rejections
> and booting with this patch removed on top of Linus' head the oops on
> shutdown goes away.

Thanks!

It looks the like there is a bug in ip_send_unicast_reply() which uses a
inet_sock/sock struct which does not have the LSM data properly initialized.

I'll put together a patch shortly.

--
paul moore
http://www.paul-moore.com

2012-08-08 19:29:47

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Wed, 2012-08-08 at 12:14 -0700, John Stultz wrote:
> On 08/07/2012 03:37 PM, John Stultz wrote:
> > On 08/07/2012 03:17 PM, Serge E. Hallyn wrote:
> >> Quoting Paul Moore ([email protected]):
> >>> On Tue, Aug 7, 2012 at 5:58 PM, John Stultz <[email protected]>
> >>> wrote:
> >>>> On 08/07/2012 02:50 PM, Paul Moore wrote:
> >>>>> On Tue, Aug 7, 2012 at 2:12 PM, John Stultz <[email protected]>
> >>>>> wrote:
> >>>>>> Hi,
> >>>>>> With my kvm environment using 3.6-rc1+, I'm seeing NULL
> >>>>>> pointer
> >>>>>> dereferences in selinux_ip_postroute_compat(). It looks like the
> >>>>>> sksec
> >>>>>> value
> >>>>>> is null and we die in the following line:
> >>>>>>
> >>>>>> if (selinux_xfrm_postroute_last(sksec->sid, skb, &ad, proto))
> >>>>>>
> >>>>>> This triggers every time I shutdown the machine, but has also
> >>>>>> triggered
> >>>>>> randomly after a few hours.
> [snip]
> >> The problem seems to be that selinux_nf_ip_init() was called, which
> >> registers the selinux_ipv4_ops (and ipv6). Those should not get
> >> registered
> >> if selinux ends up not being loaded (as in, if apparmor is loaded
> >> first),
> >> since as you've found here the selinux lsm hooks won't be called to set
> >> call selinux_sk_alloc_security().
> > This sounds about right:
> > root@testvm:~# dmesg | grep SELinux
> > [ 0.004578] SELinux: Initializing.
> > [ 0.005704] SELinux: Starting in permissive mode
> > [ 2.235034] SELinux: Registering netfilter hooks
> >
> >> I assume what's happening is that
> >> CONFIG_SECURITY_SELINUX_BOOTPARAM_VALUE was
> >> set to 1, but selinux ended up being set to disabled after the
> >> __initcall(selinux_nf_ip_init) ran? Weird.
> > This looks right as well:
> >
> > # zcat config.gz | grep SELINUX
> > CONFIG_SECURITY_SELINUX=y
> > CONFIG_SECURITY_SELINUX_BOOTPARAM=y
> > CONFIG_SECURITY_SELINUX_BOOTPARAM_VALUE=1
> > CONFIG_SECURITY_SELINUX_DISABLE=y
> > CONFIG_SECURITY_SELINUX_DEVELOP=y
> > CONFIG_SECURITY_SELINUX_AVC_STATS=y
> > CONFIG_SECURITY_SELINUX_CHECKREQPROT_VALUE=1
> > # CONFIG_SECURITY_SELINUX_POLICYDB_VERSION_MAX is not set
> > CONFIG_DEFAULT_SECURITY_SELINUX=y
> >
> >
> > Since the problem isn't completely obvious, I'm starting a bisection
> > to narrow this down some more.
>
> So I bisected this down and it seems to be the following commit:
>
> commit be9f4a44e7d41cee50ddb5f038fc2391cbbb4046
> Author: Eric Dumazet <[email protected]>
> Date: Thu Jul 19 07:34:03 2012 +0000
>
> ipv4: tcp: remove per net tcp_sock
>
>
> It doesn't revert totally cleanly, but after fixing up the rejections
> and booting with this patch removed on top of Linus' head the oops on
> shutdown goes away.

Thanks for doing this.

So sk_security is NULL and selinux crashes on it.

I guess I need to call security_sk_alloc().

2012-08-08 19:38:28

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Wed, 2012-08-08 at 15:26 -0400, Paul Moore wrote:
> On Wednesday, August 08, 2012 12:14:42 PM John Stultz wrote:
> > So I bisected this down and it seems to be the following commit:
> >
> > commit be9f4a44e7d41cee50ddb5f038fc2391cbbb4046
> > Author: Eric Dumazet <[email protected]>
> > Date: Thu Jul 19 07:34:03 2012 +0000
> >
> > ipv4: tcp: remove per net tcp_sock
> >
> >
> > It doesn't revert totally cleanly, but after fixing up the rejections
> > and booting with this patch removed on top of Linus' head the oops on
> > shutdown goes away.
>
> Thanks!
>
> It looks the like there is a bug in ip_send_unicast_reply() which uses a
> inet_sock/sock struct which does not have the LSM data properly initialized.
>
> I'll put together a patch shortly.
>

Something like this ?

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index ba39a52..027a331 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1524,6 +1524,10 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
sk->sk_priority = skb->priority;
sk->sk_protocol = ip_hdr(skb)->protocol;
sk->sk_bound_dev_if = arg->bound_dev_if;
+#ifdef CONFIG_SECURITY
+ if (!sk->sk_security && security_sk_alloc(sk, PF_INET, GFP_ATOMIC))
+ goto out;
+#endif
sock_net_set(sk, net);
__skb_queue_head_init(&sk->sk_write_queue);
sk->sk_sndbuf = sysctl_wmem_default;
@@ -1539,7 +1543,7 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
skb_set_queue_mapping(nskb, skb_get_queue_mapping(skb));
ip_push_pending_frames(sk, &fl4);
}
-
+out:
put_cpu_var(unicast_sock);

ip_rt_put(rt);

2012-08-08 19:50:26

by john stultz

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On 08/08/2012 12:38 PM, Eric Dumazet wrote:
> On Wed, 2012-08-08 at 15:26 -0400, Paul Moore wrote:
>> It looks the like there is a bug in ip_send_unicast_reply() which uses a
>> inet_sock/sock struct which does not have the LSM data properly initialized.
>>
>> I'll put together a patch shortly.
> Something like this ?
>
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index ba39a52..027a331 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1524,6 +1524,10 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
> sk->sk_priority = skb->priority;
> sk->sk_protocol = ip_hdr(skb)->protocol;
> sk->sk_bound_dev_if = arg->bound_dev_if;
> +#ifdef CONFIG_SECURITY
> + if (!sk->sk_security && security_sk_alloc(sk, PF_INET, GFP_ATOMIC))
> + goto out;
> +#endif
> sock_net_set(sk, net);
> __skb_queue_head_init(&sk->sk_write_queue);
> sk->sk_sndbuf = sysctl_wmem_default;
> @@ -1539,7 +1543,7 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
> skb_set_queue_mapping(nskb, skb_get_queue_mapping(skb));
> ip_push_pending_frames(sk, &fl4);
> }
> -
> +out:
> put_cpu_var(unicast_sock);
>
> ip_rt_put(rt);

I can't comment on the patch itself, but I tested it against Linus' HEAD
and it seems to resolve the oops on shutdown for me.

thanks
-john

2012-08-08 19:50:53

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Wednesday, August 08, 2012 09:38:21 PM Eric Dumazet wrote:
> On Wed, 2012-08-08 at 15:26 -0400, Paul Moore wrote:
> > On Wednesday, August 08, 2012 12:14:42 PM John Stultz wrote:
> > > So I bisected this down and it seems to be the following commit:
> > >
> > > commit be9f4a44e7d41cee50ddb5f038fc2391cbbb4046
> > > Author: Eric Dumazet <[email protected]>
> > > Date: Thu Jul 19 07:34:03 2012 +0000
> > >
> > > ipv4: tcp: remove per net tcp_sock
> > >
> > > It doesn't revert totally cleanly, but after fixing up the rejections
> > > and booting with this patch removed on top of Linus' head the oops on
> > > shutdown goes away.
> >
> > Thanks!
> >
> > It looks the like there is a bug in ip_send_unicast_reply() which uses a
> > inet_sock/sock struct which does not have the LSM data properly
> > initialized.
> >
> > I'll put together a patch shortly.
>
> Something like this ?

Yep. I was just trying to see if there was a way we could avoid having to
make it conditional on CONFIG_SECURITY, but I think this is a better approach
than the alternatives.

I'm also looking into making sure we get a sane LSM label on the per-cpu sock
as security_sk_alloc() just allocates and initializes the LSM blob to a basic
starting value (unlabeled_t in the case of SELinux) ... that is likely to be
the tricky bit.

Regardless, I'm okay with us merging the patch below now to fix the panic and
I'll supply a follow-up patch to fix the labeling once I figure out a solution
that seems reasonable. Does that work for you? David?

Acked-by: Paul Moore <[email protected]>

> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index ba39a52..027a331 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1524,6 +1524,10 @@ void ip_send_unicast_reply(struct net *net, struct
> sk_buff *skb, __be32 daddr, sk->sk_priority = skb->priority;
> sk->sk_protocol = ip_hdr(skb)->protocol;
> sk->sk_bound_dev_if = arg->bound_dev_if;
> +#ifdef CONFIG_SECURITY
> + if (!sk->sk_security && security_sk_alloc(sk, PF_INET, GFP_ATOMIC))
> + goto out;
> +#endif
> sock_net_set(sk, net);
> __skb_queue_head_init(&sk->sk_write_queue);
> sk->sk_sndbuf = sysctl_wmem_default;
> @@ -1539,7 +1543,7 @@ void ip_send_unicast_reply(struct net *net, struct
> sk_buff *skb, __be32 daddr, skb_set_queue_mapping(nskb,
> skb_get_queue_mapping(skb));
> ip_push_pending_frames(sk, &fl4);
> }
> -
> +out:
> put_cpu_var(unicast_sock);
>
> ip_rt_put(rt);

--
paul moore
http://www.paul-moore.com

2012-08-08 19:59:34

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Wed, Aug 8, 2012 at 3:38 PM, Eric Dumazet <[email protected]> wrote:
> On Wed, 2012-08-08 at 15:26 -0400, Paul Moore wrote:

> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index ba39a52..027a331 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1524,6 +1524,10 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
> sk->sk_priority = skb->priority;
> sk->sk_protocol = ip_hdr(skb)->protocol;
> sk->sk_bound_dev_if = arg->bound_dev_if;
> +#ifdef CONFIG_SECURITY
> + if (!sk->sk_security && security_sk_alloc(sk, PF_INET, GFP_ATOMIC))
> + goto out;
> +#endif
> sock_net_set(sk, net);
> __skb_queue_head_init(&sk->sk_write_queue);
> sk->sk_sndbuf = sysctl_wmem_default;
> @@ -1539,7 +1543,7 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
> skb_set_queue_mapping(nskb, skb_get_queue_mapping(skb));
> ip_push_pending_frames(sk, &fl4);
> }
> -
> +out:
> put_cpu_var(unicast_sock);
>
> ip_rt_put(rt);

Seems wrong. We shouldn't ever need ifdef CONFIG_SECURITY in core
code. Ifndef CONF_SECURITY then security_sk_alloc() is a static
inline return 0; I guess the question is "Where did the sk come
from"? Why wasn't security_sk_alloc() called when it was allocated?
Should it have been updated at some time and that wasn't done either?
Seems wrong to be putting packets on the queue for a socket where the
security data was never allocated and was never set to its proper
state.

there must be a bigger bug here...

-Eric

2012-08-08 20:04:10

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Wed, 2012-08-08 at 15:50 -0400, Paul Moore wrote:

> Yep. I was just trying to see if there was a way we could avoid having to
> make it conditional on CONFIG_SECURITY, but I think this is a better approach
> than the alternatives.
>
> I'm also looking into making sure we get a sane LSM label on the per-cpu sock
> as security_sk_alloc() just allocates and initializes the LSM blob to a basic
> starting value (unlabeled_t in the case of SELinux) ... that is likely to be
> the tricky bit.

It seems previous code did the same thing in sk_prot_alloc() ?

>
> Regardless, I'm okay with us merging the patch below now to fix the panic and
> I'll supply a follow-up patch to fix the labeling once I figure out a solution
> that seems reasonable. Does that work for you? David?
>
> Acked-by: Paul Moore <[email protected]>

John, could you confirm this fixes the problem ?

2012-08-08 20:04:58

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Wed, 2012-08-08 at 12:49 -0700, John Stultz wrote:

> I can't comment on the patch itself, but I tested it against Linus' HEAD
> and it seems to resolve the oops on shutdown for me.

OK, thanks !

2012-08-08 20:09:45

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Wed, 2012-08-08 at 15:59 -0400, Eric Paris wrote:

> Seems wrong. We shouldn't ever need ifdef CONFIG_SECURITY in core
> code.

Sure but it seems include file misses an accessor for this.

We could add it on a future cleanup patch, as Paul mentioned.

> Ifndef CONF_SECURITY then security_sk_alloc() is a static
> inline return 0; I guess the question is "Where did the sk come
> from"? Why wasn't security_sk_alloc() called when it was allocated?
> Should it have been updated at some time and that wasn't done either?
> Seems wrong to be putting packets on the queue for a socket where the
> security data was never allocated and was never set to its proper
> state.
>

IMHO it seems wrong to even care about security for internal sockets.

They are per cpu, shared for all users on the machine.

What kind of security do you envision exactly ?

These unicast_sock are percpu, and preallocated.

/*
* Generic function to send a packet as reply to another packet.
* Used to send some TCP resets/acks so far.
*
* Use a fake percpu inet socket to avoid false sharing and contention.
*/
static DEFINE_PER_CPU(struct inet_sock, unicast_sock) = {
.sk = {
.__sk_common = {
.skc_refcnt = ATOMIC_INIT(1),
},
.sk_wmem_alloc = ATOMIC_INIT(1),
.sk_allocation = GFP_ATOMIC,
.sk_flags = (1UL << SOCK_USE_WRITE_QUEUE),
},
.pmtudisc = IP_PMTUDISC_WANT,
.uc_ttl = -1,
};

2012-08-08 20:33:06

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Wed, 2012-08-08 at 22:09 +0200, Eric Dumazet wrote:
> On Wed, 2012-08-08 at 15:59 -0400, Eric Paris wrote:
>
> > Seems wrong. We shouldn't ever need ifdef CONFIG_SECURITY in core
> > code.
>
> Sure but it seems include file misses an accessor for this.
>
> We could add it on a future cleanup patch, as Paul mentioned.

I cooked following patch.
But smack/smack_lsm.c makes a reference to
smk_of_current()... so it seems we are in a hole...

It makes little sense to me to have any kind of security on this
internal sockets.

Maybe selinux should not crash if sk->sk_security is NULL ?

include/linux/security.h | 6 +++---
net/core/sock.c | 2 +-
net/ipv4/ip_output.c | 4 +++-
security/security.c | 4 ++--
security/selinux/hooks.c | 5 ++++-
security/smack/smack_lsm.c | 5 ++++-
6 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index 4e5a73c..aa648b2 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1601,7 +1601,7 @@ struct security_operations {
int (*socket_sock_rcv_skb) (struct sock *sk, struct sk_buff *skb);
int (*socket_getpeersec_stream) (struct socket *sock, char __user *optval, int __user *optlen, unsigned len);
int (*socket_getpeersec_dgram) (struct socket *sock, struct sk_buff *skb, u32 *secid);
- int (*sk_alloc_security) (struct sock *sk, int family, gfp_t priority);
+ int (*sk_alloc_security) (struct sock *sk, int family, gfp_t priority, bool check);
void (*sk_free_security) (struct sock *sk);
void (*sk_clone_security) (const struct sock *sk, struct sock *newsk);
void (*sk_getsecid) (struct sock *sk, u32 *secid);
@@ -2539,7 +2539,7 @@ int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb);
int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
int __user *optlen, unsigned len);
int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid);
-int security_sk_alloc(struct sock *sk, int family, gfp_t priority);
+int security_sk_alloc(struct sock *sk, int family, gfp_t priority, bool check);
void security_sk_free(struct sock *sk);
void security_sk_clone(const struct sock *sk, struct sock *newsk);
void security_sk_classify_flow(struct sock *sk, struct flowi *fl);
@@ -2667,7 +2667,7 @@ static inline int security_socket_getpeersec_dgram(struct socket *sock, struct s
return -ENOPROTOOPT;
}

-static inline int security_sk_alloc(struct sock *sk, int family, gfp_t priority)
+static inline int security_sk_alloc(struct sock *sk, int family, gfp_t priority, bool check)
{
return 0;
}
diff --git a/net/core/sock.c b/net/core/sock.c
index 8f67ced..e00cadf 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1186,7 +1186,7 @@ static struct sock *sk_prot_alloc(struct proto *prot, gfp_t priority,
if (sk != NULL) {
kmemcheck_annotate_bitfield(sk, flags);

- if (security_sk_alloc(sk, family, priority))
+ if (security_sk_alloc(sk, family, priority, false))
goto out_free;

if (!try_module_get(prot->owner))
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 76dde25..b233d6e 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1524,6 +1524,8 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
sk->sk_priority = skb->priority;
sk->sk_protocol = ip_hdr(skb)->protocol;
sk->sk_bound_dev_if = arg->bound_dev_if;
+ if (security_sk_alloc(sk, PF_INET, GFP_ATOMIC, true))
+ goto out;
sock_net_set(sk, net);
__skb_queue_head_init(&sk->sk_write_queue);
sk->sk_sndbuf = sysctl_wmem_default;
@@ -1539,7 +1541,7 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
skb_set_queue_mapping(nskb, skb_get_queue_mapping(skb));
ip_push_pending_frames(sk, &fl4);
}
-
+out:
put_cpu_var(unicast_sock);

ip_rt_put(rt);
diff --git a/security/security.c b/security/security.c
index 860aeb3..af7404e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1146,9 +1146,9 @@ int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u
}
EXPORT_SYMBOL(security_socket_getpeersec_dgram);

-int security_sk_alloc(struct sock *sk, int family, gfp_t priority)
+int security_sk_alloc(struct sock *sk, int family, gfp_t priority, bool check)
{
- return security_ops->sk_alloc_security(sk, family, priority);
+ return security_ops->sk_alloc_security(sk, family, priority, check);
}

void security_sk_free(struct sock *sk)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 6c77f63..459eca6 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4289,10 +4289,13 @@ out:
return 0;
}

-static int selinux_sk_alloc_security(struct sock *sk, int family, gfp_t priority)
+static int selinux_sk_alloc_security(struct sock *sk, int family, gfp_t priority, bool check)
{
struct sk_security_struct *sksec;

+ if (check && sk->sk_security)
+ return 0;
+
sksec = kzalloc(sizeof(*sksec), priority);
if (!sksec)
return -ENOMEM;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 8221514..8965cf1 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1754,11 +1754,14 @@ static void smack_task_to_inode(struct task_struct *p, struct inode *inode)
*
* Returns 0 on success, -ENOMEM is there's no memory
*/
-static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t gfp_flags)
+static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t gfp_flags, bool check)
{
char *csp = smk_of_current();
struct socket_smack *ssp;

+ if (check && sk->sk_security)
+ return 0;
+
ssp = kzalloc(sizeof(struct socket_smack), gfp_flags);
if (ssp == NULL)
return -ENOMEM;

2012-08-08 20:35:14

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Wednesday, August 08, 2012 10:09:38 PM Eric Dumazet wrote:
> On Wed, 2012-08-08 at 15:59 -0400, Eric Paris wrote:
> > Seems wrong. We shouldn't ever need ifdef CONFIG_SECURITY in core
> > code.
>
> Sure but it seems include file misses an accessor for this.
>
> We could add it on a future cleanup patch, as Paul mentioned.

Actually, the issue is that the shared socket doesn't have an init/alloc
function to do the LSM allocation like we do with other sockets so Eric's
patch does it as part of ip_send_unicast_reply().

If we look at the relevant part of Eric's patch:

+#ifdef CONFIG_SECURITY
+ if (!sk->sk_security && security_sk_alloc(sk, PF_INET, GFP_ATOMIC))
+ goto out;
+#endif

... if we were to remove the CONFIG_SECURITY conditional we would end up
calling security_sk_alloc() each time through in the CONFIG_SECURITY=n case as
sk->sk_security would never be initialized to a non-NULL value. In the
CONFIG_SECURITY=y case it should only be called once as security_sk_alloc()
should set sk->sk_security to a LSM blob.

> > Ifndef CONF_SECURITY then security_sk_alloc() is a static
> >
> > inline return 0; I guess the question is "Where did the sk come
> > from"? Why wasn't security_sk_alloc() called when it was allocated?
> > Should it have been updated at some time and that wasn't done either?
> > Seems wrong to be putting packets on the queue for a socket where the
> > security data was never allocated and was never set to its proper
> > state.
>
> IMHO it seems wrong to even care about security for internal sockets.
>
> They are per cpu, shared for all users on the machine.

The issue, from a security point of view, is that these sockets are sending
network traffic; even if it is just resets and timewait ACKs, it is still
network traffic and the LSMs need to be able to enforce security policy on
this traffic. After all, what would you say if your firewall let these same
packets pass without any filtering?

The issue I'm struggling with at present is how should we handle this traffic
from a LSM perspective. The label based LSMs, e.g. SELinux and Smack, use the
LSM blob assigned to locally generated outbound traffic to identify the
traffic and apply the security policy, so not only do we have to resolve the
issue of ensuring the traffic is labeled correctly, we have to do it with a
shared socket (although the patch didn't change the shared nature of the
socket).

For those who are interested, I think the reasonable labeling solution here is
to go with SECINITSID_KERNEL/kernel_t for SELinux and likely the ambient label
for Smack as in both the TCP reset and timewait ACK there shouldn't be any
actual user data present.

--
paul moore
http://www.paul-moore.com

2012-08-08 20:47:08

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Wednesday, August 08, 2012 10:32:52 PM Eric Dumazet wrote:
> On Wed, 2012-08-08 at 22:09 +0200, Eric Dumazet wrote:
> > On Wed, 2012-08-08 at 15:59 -0400, Eric Paris wrote:
> > > Seems wrong. We shouldn't ever need ifdef CONFIG_SECURITY in core
> > > code.
> >
> > Sure but it seems include file misses an accessor for this.
> >
> > We could add it on a future cleanup patch, as Paul mentioned.
>
> I cooked following patch.
> But smack/smack_lsm.c makes a reference to
> smk_of_current()... so it seems we are in a hole...
>
> It makes little sense to me to have any kind of security on this
> internal sockets.
>
> Maybe selinux should not crash if sk->sk_security is NULL ?

I realize our last emails probably passed each other mid-flight, but hopefully
it explains why we can't just pass packets when sk->sk_security is NULL.

Regardless, some quick comments below ...

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 6c77f63..459eca6 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4289,10 +4289,13 @@ out:
> return 0;
> }
>
> -static int selinux_sk_alloc_security(struct sock *sk, int family, ...
> +static int selinux_sk_alloc_security(struct sock *sk, int family, ...
> {
> struct sk_security_struct *sksec;
>
> + if (check && sk->sk_security)
> + return 0;
> +
> sksec = kzalloc(sizeof(*sksec), priority);
> if (!sksec)
> return -ENOMEM;

I think I might replace the "check" boolean with a "kern/kernel" boolean so
that in addition to the allocation we can also initialize the socket to
SECINITSID_KERNEL/kernel_t here in the case when the boolean is set. The only
place that would set the boolean to true would be ip_send_unicast_reply(), all
other callers would set it to false.

> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 8221514..8965cf1 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1754,11 +1754,14 @@ static void smack_task_to_inode(struct task_struct
> *p, struct inode *inode) *
> * Returns 0 on success, -ENOMEM is there's no memory
> */
> -static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t
> gfp_flags) +static int smack_sk_alloc_security(struct sock *sk, int family,
> gfp_t gfp_flags, bool check) {
> char *csp = smk_of_current();
> struct socket_smack *ssp;
>
> + if (check && sk->sk_security)
> + return 0;
> +
> ssp = kzalloc(sizeof(struct socket_smack), gfp_flags);
> if (ssp == NULL)
> return -ENOMEM;

In the case of Smack, when the kernel boolean is true I think the right
solution is to use smack_net_ambient.

--
paul moore
http://www.paul-moore.com

2012-08-08 20:51:58

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Wed, Aug 8, 2012 at 4:35 PM, Paul Moore <[email protected]> wrote:
> On Wednesday, August 08, 2012 10:09:38 PM Eric Dumazet wrote:

> Actually, the issue is that the shared socket doesn't have an init/alloc
> function to do the LSM allocation like we do with other sockets so Eric's
> patch does it as part of ip_send_unicast_reply().
>
> If we look at the relevant part of Eric's patch:
>
> +#ifdef CONFIG_SECURITY
> + if (!sk->sk_security && security_sk_alloc(sk, PF_INET, GFP_ATOMIC))
> + goto out;
> +#endif
>
> ... if we were to remove the CONFIG_SECURITY conditional we would end up
> calling security_sk_alloc() each time through in the CONFIG_SECURITY=n case as
> sk->sk_security would never be initialized to a non-NULL value. In the
> CONFIG_SECURITY=y case it should only be called once as security_sk_alloc()
> should set sk->sk_security to a LSM blob.

Ifndef SECURITY this turns into (because security_sk_alloc is a static
inline in that case)

if (!sk->sk_security && 0)
goto out;

Which I'd hope the compiler would optimize. So that only leaves us
caring about the case there CONFIG_SECURITY is true. In that case if
we need code which does if !alloc'd then alloc it seems we broke the
model of everything else in the code and added a branch needlessly.

Could we add a __init function which does the security_sk_alloc() in
the same file where we declared them?

>> IMHO it seems wrong to even care about security for internal sockets.
>>
>> They are per cpu, shared for all users on the machine.
>
> The issue, from a security point of view, is that these sockets are sending
> network traffic; even if it is just resets and timewait ACKs, it is still
> network traffic and the LSMs need to be able to enforce security policy on
> this traffic. After all, what would you say if your firewall let these same
> packets pass without any filtering?
>
> The issue I'm struggling with at present is how should we handle this traffic
> from a LSM perspective. The label based LSMs, e.g. SELinux and Smack, use the
> LSM blob assigned to locally generated outbound traffic to identify the
> traffic and apply the security policy, so not only do we have to resolve the
> issue of ensuring the traffic is labeled correctly, we have to do it with a
> shared socket (although the patch didn't change the shared nature of the
> socket).
>
> For those who are interested, I think the reasonable labeling solution here is
> to go with SECINITSID_KERNEL/kernel_t for SELinux and likely the ambient label
> for Smack as in both the TCP reset and timewait ACK there shouldn't be any
> actual user data present.

I'm willing to accept that argument from an SELinux perspective. I'd
also accept the argument that it is private and do something similar
to what we do with IS_PRIVATE on inodes. Although sockets probably
don't have a good field to use...

2012-08-08 21:03:24

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Wednesday, August 08, 2012 04:51:56 PM Eric Paris wrote:
> On Wed, Aug 8, 2012 at 4:35 PM, Paul Moore <[email protected]> wrote:
> > On Wednesday, August 08, 2012 10:09:38 PM Eric Dumazet wrote:
> >
> > Actually, the issue is that the shared socket doesn't have an init/alloc
> > function to do the LSM allocation like we do with other sockets so Eric's
> > patch does it as part of ip_send_unicast_reply().
> >
> > If we look at the relevant part of Eric's patch:
> > +#ifdef CONFIG_SECURITY
> > + if (!sk->sk_security && security_sk_alloc(sk, PF_INET,
> > GFP_ATOMIC))
> > + goto out;
> > +#endif
> >
> > ... if we were to remove the CONFIG_SECURITY conditional we would end up
> > calling security_sk_alloc() each time through in the CONFIG_SECURITY=n
> > case as sk->sk_security would never be initialized to a non-NULL value.
> > In the CONFIG_SECURITY=y case it should only be called once as
> > security_sk_alloc() should set sk->sk_security to a LSM blob.
>
> Ifndef SECURITY this turns into (because security_sk_alloc is a static
> inline in that case)
>
> if (!sk->sk_security && 0)
> goto out;
>
> Which I'd hope the compiler would optimize. So that only leaves us
> caring about the case there CONFIG_SECURITY is true. In that case if
> we need code which does if !alloc'd then alloc it seems we broke the
> model of everything else in the code and added a branch needlessly.
>
> Could we add a __init function which does the security_sk_alloc() in
> the same file where we declared them?

Is it safe to call security_sk_alloc() from inside another __init function? I
think in both the case of SELinux and Smack it shouldn't be a problem, but I'm
concerned about the more general case of calling a LSM hook potentially before
the LSM has been initialized.

If that isn't an issue we could probably do something in ip_init().

> > The issue I'm struggling with at present is how should we handle this
> > traffic from a LSM perspective. The label based LSMs, e.g. SELinux and
> > Smack, use the LSM blob assigned to locally generated outbound traffic to
> > identify the traffic and apply the security policy, so not only do we
> > have to resolve the issue of ensuring the traffic is labeled correctly,
> > we have to do it with a shared socket (although the patch didn't change
> > the shared nature of the socket).
> >
> > For those who are interested, I think the reasonable labeling solution
> > here is to go with SECINITSID_KERNEL/kernel_t for SELinux and likely the
> > ambient label for Smack as in both the TCP reset and timewait ACK there
> > shouldn't be any actual user data present.
>
> I'm willing to accept that argument from an SELinux perspective. I'd
> also accept the argument that it is private and do something similar
> to what we do with IS_PRIVATE on inodes. Although sockets probably
> don't have a good field to use...

I'm not aware of one. See my comments on Eric's last patch posting (the other
Eric, not you).

--
paul moore
http://www.paul-moore.com

2012-08-08 21:09:22

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Wed, Aug 8, 2012 at 5:03 PM, Paul Moore <[email protected]> wrote:
> On Wednesday, August 08, 2012 04:51:56 PM Eric Paris wrote:

>> Could we add a __init function which does the security_sk_alloc() in
>> the same file where we declared them?
>
> Is it safe to call security_sk_alloc() from inside another __init function? I
> think in both the case of SELinux and Smack it shouldn't be a problem, but I'm
> concerned about the more general case of calling a LSM hook potentially before
> the LSM has been initialized.
>
> If that isn't an issue we could probably do something in ip_init().

The security_initcall() functions should happen way before __init
functions. If an LSM busts, it's the LSM initializing itself too late
not the code here being wrong...

2012-08-08 21:54:55

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Wed, 2012-08-08 at 16:46 -0400, Paul Moore wrote:
> On Wednesday, August 08, 2012 10:32:52 PM Eric Dumazet wrote:
> > On Wed, 2012-08-08 at 22:09 +0200, Eric Dumazet wrote:
> > > On Wed, 2012-08-08 at 15:59 -0400, Eric Paris wrote:
> > > > Seems wrong. We shouldn't ever need ifdef CONFIG_SECURITY in core
> > > > code.
> > >
> > > Sure but it seems include file misses an accessor for this.
> > >
> > > We could add it on a future cleanup patch, as Paul mentioned.
> >
> > I cooked following patch.
> > But smack/smack_lsm.c makes a reference to
> > smk_of_current()... so it seems we are in a hole...
> >
> > It makes little sense to me to have any kind of security on this
> > internal sockets.
> >
> > Maybe selinux should not crash if sk->sk_security is NULL ?
>
> I realize our last emails probably passed each other mid-flight, but hopefully
> it explains why we can't just pass packets when sk->sk_security is NULL.
>
> Regardless, some quick comments below ...
>
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 6c77f63..459eca6 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -4289,10 +4289,13 @@ out:
> > return 0;
> > }
> >
> > -static int selinux_sk_alloc_security(struct sock *sk, int family, ...
> > +static int selinux_sk_alloc_security(struct sock *sk, int family, ...
> > {
> > struct sk_security_struct *sksec;
> >
> > + if (check && sk->sk_security)
> > + return 0;
> > +
> > sksec = kzalloc(sizeof(*sksec), priority);
> > if (!sksec)
> > return -ENOMEM;
>
> I think I might replace the "check" boolean with a "kern/kernel" boolean so
> that in addition to the allocation we can also initialize the socket to
> SECINITSID_KERNEL/kernel_t here in the case when the boolean is set. The only
> place that would set the boolean to true would be ip_send_unicast_reply(), all
> other callers would set it to false.
>
> > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> > index 8221514..8965cf1 100644
> > --- a/security/smack/smack_lsm.c
> > +++ b/security/smack/smack_lsm.c
> > @@ -1754,11 +1754,14 @@ static void smack_task_to_inode(struct task_struct
> > *p, struct inode *inode) *
> > * Returns 0 on success, -ENOMEM is there's no memory
> > */
> > -static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t
> > gfp_flags) +static int smack_sk_alloc_security(struct sock *sk, int family,
> > gfp_t gfp_flags, bool check) {
> > char *csp = smk_of_current();
> > struct socket_smack *ssp;
> >
> > + if (check && sk->sk_security)
> > + return 0;
> > +
> > ssp = kzalloc(sizeof(struct socket_smack), gfp_flags);
> > if (ssp == NULL)
> > return -ENOMEM;
>
> In the case of Smack, when the kernel boolean is true I think the right
> solution is to use smack_net_ambient.
>

cool, here the last version :

diff --git a/include/linux/security.h b/include/linux/security.h
index 4e5a73c..4d8e454 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1601,7 +1601,7 @@ struct security_operations {
int (*socket_sock_rcv_skb) (struct sock *sk, struct sk_buff *skb);
int (*socket_getpeersec_stream) (struct socket *sock, char __user *optval, int __user *optlen, unsigned len);
int (*socket_getpeersec_dgram) (struct socket *sock, struct sk_buff *skb, u32 *secid);
- int (*sk_alloc_security) (struct sock *sk, int family, gfp_t priority);
+ int (*sk_alloc_security) (struct sock *sk, int family, gfp_t priority, bool kernel);
void (*sk_free_security) (struct sock *sk);
void (*sk_clone_security) (const struct sock *sk, struct sock *newsk);
void (*sk_getsecid) (struct sock *sk, u32 *secid);
@@ -2539,7 +2539,7 @@ int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb);
int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
int __user *optlen, unsigned len);
int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid);
-int security_sk_alloc(struct sock *sk, int family, gfp_t priority);
+int security_sk_alloc(struct sock *sk, int family, gfp_t priority, bool kernel);
void security_sk_free(struct sock *sk);
void security_sk_clone(const struct sock *sk, struct sock *newsk);
void security_sk_classify_flow(struct sock *sk, struct flowi *fl);
@@ -2667,7 +2667,7 @@ static inline int security_socket_getpeersec_dgram(struct socket *sock, struct s
return -ENOPROTOOPT;
}

-static inline int security_sk_alloc(struct sock *sk, int family, gfp_t priority)
+static inline int security_sk_alloc(struct sock *sk, int family, gfp_t priority, bool kernel)
{
return 0;
}
diff --git a/net/core/sock.c b/net/core/sock.c
index 8f67ced..e00cadf 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1186,7 +1186,7 @@ static struct sock *sk_prot_alloc(struct proto *prot, gfp_t priority,
if (sk != NULL) {
kmemcheck_annotate_bitfield(sk, flags);

- if (security_sk_alloc(sk, family, priority))
+ if (security_sk_alloc(sk, family, priority, false))
goto out_free;

if (!try_module_get(prot->owner))
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 76dde25..b233d6e 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1524,6 +1524,8 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
sk->sk_priority = skb->priority;
sk->sk_protocol = ip_hdr(skb)->protocol;
sk->sk_bound_dev_if = arg->bound_dev_if;
+ if (security_sk_alloc(sk, PF_INET, GFP_ATOMIC, true))
+ goto out;
sock_net_set(sk, net);
__skb_queue_head_init(&sk->sk_write_queue);
sk->sk_sndbuf = sysctl_wmem_default;
@@ -1539,7 +1541,7 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
skb_set_queue_mapping(nskb, skb_get_queue_mapping(skb));
ip_push_pending_frames(sk, &fl4);
}
-
+out:
put_cpu_var(unicast_sock);

ip_rt_put(rt);
diff --git a/security/security.c b/security/security.c
index 860aeb3..23cf297 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1146,9 +1146,9 @@ int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u
}
EXPORT_SYMBOL(security_socket_getpeersec_dgram);

-int security_sk_alloc(struct sock *sk, int family, gfp_t priority)
+int security_sk_alloc(struct sock *sk, int family, gfp_t priority, bool kernel)
{
- return security_ops->sk_alloc_security(sk, family, priority);
+ return security_ops->sk_alloc_security(sk, family, priority, kernel);
}

void security_sk_free(struct sock *sk)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 6c77f63..ccd4374 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4289,10 +4289,13 @@ out:
return 0;
}

-static int selinux_sk_alloc_security(struct sock *sk, int family, gfp_t priority)
+static int selinux_sk_alloc_security(struct sock *sk, int family, gfp_t priority, bool kernel)
{
struct sk_security_struct *sksec;

+ if (kernel && sk->sk_security)
+ return 0;
+
sksec = kzalloc(sizeof(*sksec), priority);
if (!sksec)
return -ENOMEM;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 8221514..207d9cc 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1749,20 +1749,25 @@ static void smack_task_to_inode(struct task_struct *p, struct inode *inode)
* @sk: the socket
* @family: unused
* @gfp_flags: memory allocation flags
+ * @kernel: true if we should check sk_security being already set
*
* Assign Smack pointers to current
*
* Returns 0 on success, -ENOMEM is there's no memory
*/
-static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t gfp_flags)
+static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t gfp_flags, bool kernel)
{
char *csp = smk_of_current();
struct socket_smack *ssp;

+ if (kernel && sk->sk_security)
+ return 0;
+
ssp = kzalloc(sizeof(struct socket_smack), gfp_flags);
if (ssp == NULL)
return -ENOMEM;
-
+ /* kernel is true if called from ip_send_unicast_reply() */
+ csp = kernel ? smack_net_ambient : smk_of_current();
ssp->smk_in = csp;
ssp->smk_out = csp;
ssp->smk_packet = NULL;

2012-08-09 00:00:31

by Casey Schaufler

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On 8/8/2012 2:54 PM, Eric Dumazet wrote:

By the way, once this proved to be an issue that involved
more than just SELinux it needed to go onto the LSM list as
well.

> On Wed, 2012-08-08 at 16:46 -0400, Paul Moore wrote:
>> On Wednesday, August 08, 2012 10:32:52 PM Eric Dumazet wrote:
>>> On Wed, 2012-08-08 at 22:09 +0200, Eric Dumazet wrote:
>>>> On Wed, 2012-08-08 at 15:59 -0400, Eric Paris wrote:
>>>>> Seems wrong. We shouldn't ever need ifdef CONFIG_SECURITY in core
>>>>> code.
>>>> Sure but it seems include file misses an accessor for this.
>>>>
>>>> We could add it on a future cleanup patch, as Paul mentioned.
>>> I cooked following patch.
>>> But smack/smack_lsm.c makes a reference to
>>> smk_of_current()... so it seems we are in a hole...
>>>
>>> It makes little sense to me to have any kind of security on this
>>> internal sockets.
>>>
>>> Maybe selinux should not crash if sk->sk_security is NULL ?
>> I realize our last emails probably passed each other mid-flight, but hopefully
>> it explains why we can't just pass packets when sk->sk_security is NULL.
>>
>> Regardless, some quick comments below ...
>>
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index 6c77f63..459eca6 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -4289,10 +4289,13 @@ out:
>>> return 0;
>>> }
>>>
>>> -static int selinux_sk_alloc_security(struct sock *sk, int family, ...
>>> +static int selinux_sk_alloc_security(struct sock *sk, int family, ...
>>> {
>>> struct sk_security_struct *sksec;
>>>
>>> + if (check && sk->sk_security)
>>> + return 0;
>>> +
>>> sksec = kzalloc(sizeof(*sksec), priority);
>>> if (!sksec)
>>> return -ENOMEM;
>> I think I might replace the "check" boolean with a "kern/kernel" boolean so
>> that in addition to the allocation we can also initialize the socket to
>> SECINITSID_KERNEL/kernel_t here in the case when the boolean is set. The only
>> place that would set the boolean to true would be ip_send_unicast_reply(), all
>> other callers would set it to false.
>>
>>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>>> index 8221514..8965cf1 100644
>>> --- a/security/smack/smack_lsm.c
>>> +++ b/security/smack/smack_lsm.c
>>> @@ -1754,11 +1754,14 @@ static void smack_task_to_inode(struct task_struct
>>> *p, struct inode *inode) *
>>> * Returns 0 on success, -ENOMEM is there's no memory
>>> */
>>> -static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t
>>> gfp_flags) +static int smack_sk_alloc_security(struct sock *sk, int family,
>>> gfp_t gfp_flags, bool check) {
>>> char *csp = smk_of_current();
>>> struct socket_smack *ssp;
>>>
>>> + if (check && sk->sk_security)
>>> + return 0;
>>> +
>>> ssp = kzalloc(sizeof(struct socket_smack), gfp_flags);
>>> if (ssp == NULL)
>>> return -ENOMEM;
>> In the case of Smack, when the kernel boolean is true I think the right
>> solution is to use smack_net_ambient.

I confess that my understanding of unicast is limited.
If the intention is to send an unlabeled packet then
indeed smack_net_ambient is the way to go.

>>
> cool, here the last version :
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 4e5a73c..4d8e454 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1601,7 +1601,7 @@ struct security_operations {
> int (*socket_sock_rcv_skb) (struct sock *sk, struct sk_buff *skb);
> int (*socket_getpeersec_stream) (struct socket *sock, char __user *optval, int __user *optlen, unsigned len);
> int (*socket_getpeersec_dgram) (struct socket *sock, struct sk_buff *skb, u32 *secid);
> - int (*sk_alloc_security) (struct sock *sk, int family, gfp_t priority);
> + int (*sk_alloc_security) (struct sock *sk, int family, gfp_t priority, bool kernel);

Is there no information already available in the sock
that will tell us this is a unicast operation?

> void (*sk_free_security) (struct sock *sk);
> void (*sk_clone_security) (const struct sock *sk, struct sock *newsk);
> void (*sk_getsecid) (struct sock *sk, u32 *secid);
> @@ -2539,7 +2539,7 @@ int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb);
> int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> int __user *optlen, unsigned len);
> int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid);
> -int security_sk_alloc(struct sock *sk, int family, gfp_t priority);
> +int security_sk_alloc(struct sock *sk, int family, gfp_t priority, bool kernel);
> void security_sk_free(struct sock *sk);
> void security_sk_clone(const struct sock *sk, struct sock *newsk);
> void security_sk_classify_flow(struct sock *sk, struct flowi *fl);
> @@ -2667,7 +2667,7 @@ static inline int security_socket_getpeersec_dgram(struct socket *sock, struct s
> return -ENOPROTOOPT;
> }
>
> -static inline int security_sk_alloc(struct sock *sk, int family, gfp_t priority)
> +static inline int security_sk_alloc(struct sock *sk, int family, gfp_t priority, bool kernel)
> {
> return 0;
> }
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 8f67ced..e00cadf 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1186,7 +1186,7 @@ static struct sock *sk_prot_alloc(struct proto *prot, gfp_t priority,
> if (sk != NULL) {
> kmemcheck_annotate_bitfield(sk, flags);
>
> - if (security_sk_alloc(sk, family, priority))
> + if (security_sk_alloc(sk, family, priority, false))
> goto out_free;
>
> if (!try_module_get(prot->owner))
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index 76dde25..b233d6e 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1524,6 +1524,8 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
> sk->sk_priority = skb->priority;
> sk->sk_protocol = ip_hdr(skb)->protocol;
> sk->sk_bound_dev_if = arg->bound_dev_if;
> + if (security_sk_alloc(sk, PF_INET, GFP_ATOMIC, true))
> + goto out;
> sock_net_set(sk, net);
> __skb_queue_head_init(&sk->sk_write_queue);
> sk->sk_sndbuf = sysctl_wmem_default;
> @@ -1539,7 +1541,7 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
> skb_set_queue_mapping(nskb, skb_get_queue_mapping(skb));
> ip_push_pending_frames(sk, &fl4);
> }
> -
> +out:
> put_cpu_var(unicast_sock);
>
> ip_rt_put(rt);
> diff --git a/security/security.c b/security/security.c
> index 860aeb3..23cf297 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1146,9 +1146,9 @@ int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u
> }
> EXPORT_SYMBOL(security_socket_getpeersec_dgram);
>
> -int security_sk_alloc(struct sock *sk, int family, gfp_t priority)
> +int security_sk_alloc(struct sock *sk, int family, gfp_t priority, bool kernel)
> {
> - return security_ops->sk_alloc_security(sk, family, priority);
> + return security_ops->sk_alloc_security(sk, family, priority, kernel);
> }
>
> void security_sk_free(struct sock *sk)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 6c77f63..ccd4374 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4289,10 +4289,13 @@ out:
> return 0;
> }
>
> -static int selinux_sk_alloc_security(struct sock *sk, int family, gfp_t priority)
> +static int selinux_sk_alloc_security(struct sock *sk, int family, gfp_t priority, bool kernel)
> {
> struct sk_security_struct *sksec;
>
> + if (kernel && sk->sk_security)
> + return 0;
> +
> sksec = kzalloc(sizeof(*sksec), priority);
> if (!sksec)
> return -ENOMEM;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 8221514..207d9cc 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1749,20 +1749,25 @@ static void smack_task_to_inode(struct task_struct *p, struct inode *inode)
> * @sk: the socket
> * @family: unused
> * @gfp_flags: memory allocation flags
> + * @kernel: true if we should check sk_security being already set
> *
> * Assign Smack pointers to current
> *
> * Returns 0 on success, -ENOMEM is there's no memory
> */
> -static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t gfp_flags)
> +static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t gfp_flags, bool kernel)
> {
> char *csp = smk_of_current();
> struct socket_smack *ssp;
>
> + if (kernel && sk->sk_security)
> + return 0;
> +
> ssp = kzalloc(sizeof(struct socket_smack), gfp_flags);
> if (ssp == NULL)
> return -ENOMEM;
> -
> + /* kernel is true if called from ip_send_unicast_reply() */
> + csp = kernel ? smack_net_ambient : smk_of_current();

How about ...

if (kernel)
csp = smack_net_ambient;

... as csp is set to smk_of_current() in the declaration.
That, or change the declaration.

> ssp->smk_in = csp;
> ssp->smk_out = csp;
> ssp->smk_packet = NULL;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

2012-08-09 13:30:50

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Wednesday, August 08, 2012 05:00:26 PM Casey Schaufler wrote:
> On 8/8/2012 2:54 PM, Eric Dumazet wrote:
>
> By the way, once this proved to be an issue that involved
> more than just SELinux it needed to go onto the LSM list as
> well.

Yes, you're right.

> > On Wed, 2012-08-08 at 16:46 -0400, Paul Moore wrote:
> >> On Wednesday, August 08, 2012 10:32:52 PM Eric Dumazet wrote:
> >>> On Wed, 2012-08-08 at 22:09 +0200, Eric Dumazet wrote:
> >>> +static int smack_sk_alloc_security(struct sock *sk, int ...
> >>> {
> >>> char *csp = smk_of_current();
> >>> struct socket_smack *ssp;
> >>>
> >>> + if (check && sk->sk_security)
> >>> + return 0;
> >>> +
> >>>
> >>> ssp = kzalloc(sizeof(struct socket_smack), gfp_flags);
> >>> if (ssp == NULL)
> >>>
> >>> return -ENOMEM;
> >>
> >> In the case of Smack, when the kernel boolean is true I think the right
> >> solution is to use smack_net_ambient.
>
> I confess that my understanding of unicast is limited.
> If the intention is to send an unlabeled packet then
> indeed smack_net_ambient is the way to go.

Well, the intention isn't necessarily to send an unlabeled packet, although
that may be the end result.

In the case of a TCP reset the kernel/ambient label it is hard to argue that
the kernel/ambient label is not the correct solution; in this case there was
never an associated socket so the kernel itself needs to respond.

In the case of a TCP syn-recv and timewait ACK things are a little less clear.
Eric (Dumazet), it looks like we have a socket in tcp_v4_reqsk_send_ack() and
tcp_v4_timewait_ack(), any reason why we can't propagate the socket down to
ip_send_unicast_reply()?

--
paul moore
http://www.paul-moore.com

2012-08-09 14:28:00

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Thu, 2012-08-09 at 09:30 -0400, Paul Moore wrote:

> In the case of a TCP syn-recv and timewait ACK things are a little less clear.
> Eric (Dumazet), it looks like we have a socket in tcp_v4_reqsk_send_ack() and
> tcp_v4_timewait_ack(), any reason why we can't propagate the socket down to
> ip_send_unicast_reply()?
>

timewait 'sockets' are not full blown sockets.

We need a socket (well, a good part of it) to build the IP frame and
send it.

2012-08-09 14:50:43

[permalink] [raw]

Subject: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock

From: Eric Dumazet <[email protected]>

commit be9f4a44e7d41cee (ipv4: tcp: remove per net tcp_sock) added a
selinux regression, reported and bisected by John Stultz

selinux_ip_postroute_compat() expect to find a valid sk->sk_security
pointer, but this field is NULL for unicast_sock

Fix this by adding a new 'kernel' parameter to security_sk_alloc(),
set to true if socket might already have a valid sk->sk_security
pointer. ip_send_unicast_reply() uses a percpu fake socket, so the first
call to security_sk_alloc() will populate sk->sk_security pointer,
subsequent ones will reuse existing context.

Reported-by: John Stultz <[email protected]>
Bisected-by: John Stultz <[email protected]>
Signed-off-by: Eric Dumazet <[email protected]>
Cc: Paul Moore <[email protected]>
Cc: Eric Paris <[email protected]>
Cc: "Serge E. Hallyn" <[email protected]>
---
include/linux/security.h | 6 +++---
net/core/sock.c | 2 +-
net/ipv4/ip_output.c | 4 +++-
security/security.c | 4 ++--
security/selinux/hooks.c | 5 ++++-
security/smack/smack_lsm.c | 10 ++++++++--
6 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index 4e5a73c..4d8e454 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1601,7 +1601,7 @@ struct security_operations {
int (*socket_sock_rcv_skb) (struct sock *sk, struct sk_buff *skb);
int (*socket_getpeersec_stream) (struct socket *sock, char __user *optval, int __user *optlen, unsigned len);
int (*socket_getpeersec_dgram) (struct socket *sock, struct sk_buff *skb, u32 *secid);
- int (*sk_alloc_security) (struct sock *sk, int family, gfp_t priority);
+ int (*sk_alloc_security) (struct sock *sk, int family, gfp_t priority, bool kernel);
void (*sk_free_security) (struct sock *sk);
void (*sk_clone_security) (const struct sock *sk, struct sock *newsk);
void (*sk_getsecid) (struct sock *sk, u32 *secid);
@@ -2539,7 +2539,7 @@ int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb);
int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
int __user *optlen, unsigned len);
int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid);
-int security_sk_alloc(struct sock *sk, int family, gfp_t priority);
+int security_sk_alloc(struct sock *sk, int family, gfp_t priority, bool kernel);
void security_sk_free(struct sock *sk);
void security_sk_clone(const struct sock *sk, struct sock *newsk);
void security_sk_classify_flow(struct sock *sk, struct flowi *fl);
@@ -2667,7 +2667,7 @@ static inline int security_socket_getpeersec_dgram(struct socket *sock, struct s
return -ENOPROTOOPT;
}

-static inline int security_sk_alloc(struct sock *sk, int family, gfp_t priority)
+static inline int security_sk_alloc(struct sock *sk, int family, gfp_t priority, bool kernel)
{
return 0;
}
diff --git a/net/core/sock.c b/net/core/sock.c
index 8f67ced..e00cadf 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1186,7 +1186,7 @@ static struct sock *sk_prot_alloc(struct proto *prot, gfp_t priority,
if (sk != NULL) {
kmemcheck_annotate_bitfield(sk, flags);

- if (security_sk_alloc(sk, family, priority))
+ if (security_sk_alloc(sk, family, priority, false))
goto out_free;

if (!try_module_get(prot->owner))
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 76dde25..b233d6e 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1524,6 +1524,8 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
sk->sk_priority = skb->priority;
sk->sk_protocol = ip_hdr(skb)->protocol;
sk->sk_bound_dev_if = arg->bound_dev_if;
+ if (security_sk_alloc(sk, PF_INET, GFP_ATOMIC, true))
+ goto out;
sock_net_set(sk, net);
__skb_queue_head_init(&sk->sk_write_queue);
sk->sk_sndbuf = sysctl_wmem_default;
@@ -1539,7 +1541,7 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
skb_set_queue_mapping(nskb, skb_get_queue_mapping(skb));
ip_push_pending_frames(sk, &fl4);
}
-
+out:
put_cpu_var(unicast_sock);

ip_rt_put(rt);
diff --git a/security/security.c b/security/security.c
index 860aeb3..23cf297 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1146,9 +1146,9 @@ int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u
}
EXPORT_SYMBOL(security_socket_getpeersec_dgram);

-int security_sk_alloc(struct sock *sk, int family, gfp_t priority)
+int security_sk_alloc(struct sock *sk, int family, gfp_t priority, bool kernel)
{
- return security_ops->sk_alloc_security(sk, family, priority);
+ return security_ops->sk_alloc_security(sk, family, priority, kernel);
}

void security_sk_free(struct sock *sk)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 6c77f63..ccd4374 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4289,10 +4289,13 @@ out:
return 0;
}

-static int selinux_sk_alloc_security(struct sock *sk, int family, gfp_t priority)
+static int selinux_sk_alloc_security(struct sock *sk, int family, gfp_t priority, bool kernel)
{
struct sk_security_struct *sksec;

+ if (kernel && sk->sk_security)
+ return 0;
+
sksec = kzalloc(sizeof(*sksec), priority);
if (!sksec)
return -ENOMEM;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 8221514..0b066d0 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1749,20 +1749,26 @@ static void smack_task_to_inode(struct task_struct *p, struct inode *inode)
* @sk: the socket
* @family: unused
* @gfp_flags: memory allocation flags
+ * @kernel: true if we should check sk_security being already set
*
* Assign Smack pointers to current
*
* Returns 0 on success, -ENOMEM is there's no memory
*/
-static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t gfp_flags)
+static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t gfp_flags, bool kernel)
{
- char *csp = smk_of_current();
+ char *csp;
struct socket_smack *ssp;

+ if (kernel && sk->sk_security)
+ return 0;
+
ssp = kzalloc(sizeof(struct socket_smack), gfp_flags);
if (ssp == NULL)
return -ENOMEM;

+ csp = kernel ? smack_net_ambient : smk_of_current();
+
ssp->smk_in = csp;
ssp->smk_out = csp;
ssp->smk_packet = NULL;

2012-08-09 15:04:21

[permalink] [raw]

Subject: Re: NULL pointer dereference in selinux_ip_postroute_compat

On Thu, Aug 9, 2012 at 10:27 AM, Eric Dumazet <[email protected]> wrote:
> On Thu, 2012-08-09 at 09:30 -0400, Paul Moore wrote:
>
>> In the case of a TCP syn-recv and timewait ACK things are a little less clear.
>> Eric (Dumazet), it looks like we have a socket in tcp_v4_reqsk_send_ack() and
>> tcp_v4_timewait_ack(), any reason why we can't propagate the socket down to
>> ip_send_unicast_reply()?
>>
>
> timewait 'sockets' are not full blown sockets.
>
> We need a socket (well, a good part of it) to build the IP frame and
> send it.

Yes, of course you're right.

Ideally we need a better solution here from a LSM perspective, but I
don't think this should hold up the fix as the labeling was broken
even before the postroute_compat() code broke.

--
paul moore
http://www.paul-moore.com

2012-08-09 15:07:24

[permalink] [raw]

Subject: Re: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock

On Thursday, August 09, 2012 04:50:33 PM Eric Dumazet wrote:
> From: Eric Dumazet <[email protected]>
>
> commit be9f4a44e7d41cee (ipv4: tcp: remove per net tcp_sock) added a
> selinux regression, reported and bisected by John Stultz
>
> selinux_ip_postroute_compat() expect to find a valid sk->sk_security
> pointer, but this field is NULL for unicast_sock
>
> Fix this by adding a new 'kernel' parameter to security_sk_alloc(),
> set to true if socket might already have a valid sk->sk_security
> pointer. ip_send_unicast_reply() uses a percpu fake socket, so the first
> call to security_sk_alloc() will populate sk->sk_security pointer,
> subsequent ones will reuse existing context.
>
> Reported-by: John Stultz <[email protected]>
> Bisected-by: John Stultz <[email protected]>
> Signed-off-by: Eric Dumazet <[email protected]>
> Cc: Paul Moore <[email protected]>
> Cc: Eric Paris <[email protected]>
> Cc: "Serge E. Hallyn" <[email protected]>

...

> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index 76dde25..b233d6e 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1524,6 +1524,8 @@ void ip_send_unicast_reply(struct net *net, struct
> sk_buff *skb, __be32 daddr, sk->sk_priority = skb->priority;
> sk->sk_protocol = ip_hdr(skb)->protocol;
> sk->sk_bound_dev_if = arg->bound_dev_if;
> + if (security_sk_alloc(sk, PF_INET, GFP_ATOMIC, true))
> + goto out;
> sock_net_set(sk, net);
> __skb_queue_head_init(&sk->sk_write_queue);
> sk->sk_sndbuf = sysctl_wmem_default;

Is is possible to do the call to security_sk_alloc() in the ip_init() function
or does the per-cpu nature of the socket make this a pain?

--
paul moore
http://www.paul-moore.com

2012-08-09 15:36:57

[permalink] [raw]

Subject: Re: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock

On Thu, 2012-08-09 at 11:07 -0400, Paul Moore wrote:

> Is is possible to do the call to security_sk_alloc() in the ip_init() function
> or does the per-cpu nature of the socket make this a pain?
>

Its a pain, if we want NUMA affinity.

Here, each cpu should get memory from its closest node.

2012-08-09 15:59:26

[permalink] [raw]

Subject: Re: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock

2012-08-09 16:05:43

[permalink] [raw]

Subject: Re: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock

On Thu, Aug 9, 2012 at 11:36 AM, Eric Dumazet <[email protected]> wrote:
> On Thu, 2012-08-09 at 11:07 -0400, Paul Moore wrote:
>
>> Is is possible to do the call to security_sk_alloc() in the ip_init() function
>> or does the per-cpu nature of the socket make this a pain?
>>
>
> Its a pain, if we want NUMA affinity.
>
> Here, each cpu should get memory from its closest node.

I really really don't like it. I won't say NAK, but it is the first
and only place in the kernel where I believe we allocate an object and
don't allocate the security blob until some random later point in
time. If it is such a performance issue to have the security blob in
the same numa node, isn't adding a number of branches and putting this
function call on every output at least as bad? Aren't we discouraged
from GFP_ATOMIC? In __init we can use GFP_KERNEL.

This still doesn't fix these sockets entirely. We now have the
security blob allocated, but it was never set to something useful.
Paul, are you looking into this? This is a bandaide, not a fix....

-Eric

2012-08-09 16:09:05

[permalink] [raw]

Subject: Re: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock

On Thu, Aug 9, 2012 at 12:05 PM, Eric Paris <[email protected]> wrote:
> Paul, are you looking into this? This is a bandaide, not a fix....

Yep, I mentioned this a few times in the other thread. The problem is
there is not going to be an easy fix for the labeling so I'd rather we
see this patch, or something like it, go in now to resolve the kernel
panic, and fix the labeling later.

--
paul moore
http://www.paul-moore.com

2012-08-09 17:46:21

[permalink] [raw]

Subject: Re: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock

On Thu, 2012-08-09 at 12:05 -0400, Eric Paris wrote:
> On Thu, Aug 9, 2012 at 11:36 AM, Eric Dumazet <[email protected]> wrote:
> > On Thu, 2012-08-09 at 11:07 -0400, Paul Moore wrote:
> >
> >> Is is possible to do the call to security_sk_alloc() in the ip_init() function
> >> or does the per-cpu nature of the socket make this a pain?
> >>
> >
> > Its a pain, if we want NUMA affinity.
> >
> > Here, each cpu should get memory from its closest node.
>
> I really really don't like it. I won't say NAK, but it is the first
> and only place in the kernel where I believe we allocate an object and
> don't allocate the security blob until some random later point in
> time.

...

> If it is such a performance issue to have the security blob in
> the same numa node, isn't adding a number of branches and putting this
> function call on every output at least as bad? Aren't we discouraged
> from GFP_ATOMIC? In __init we can use GFP_KERNEL.

What a big deal. Its done _once_ time per cpu, and this is so small blob
of memory you'll have to show us one single failure out of one million
boots.

If the security_sk_alloc() fails, we dont care. We are about sending a
RESET or ACK packet. They can be lost by the network, or even skb
allocation can fail. Nobody ever noticed and complained.

Every time we accept() a new socket (and call security_sk_alloc()), its
done under soft irq, thus GFP_ATOMIC, and you didn't complain yet, while
a socket needs about 2 Kbytes of memory...

>
> This still doesn't fix these sockets entirely. We now have the
> security blob allocated, but it was never set to something useful.
> Paul, are you looking into this? This is a bandaide, not a fix....
>

Please do so, on a followup patch, dont pretend I must fix all this
stuff.

2012-08-09 20:06:23

[permalink] [raw]

Subject: Re: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock

NAK.

I personally think commit be9f4a44e7d41cee should be reverted until it
is fixed. Let me explain what all I believe it broke and how.

Old callchain of the creation of the 'equivalent' socket previous to
the patch in question just for reference:

inet_ctl_sock_create
sock_create_kern
__sock_create
pf->create (inet_create)
sk_alloc
sk_prot_alloc
security_sk_alloc()

This WAS working properly. All of it. The equivalent struct sock was
being created and allocated in inet_create(), which called to
sk_alloc->sk_prot_alloc->security_sk_alloc(). We all agree that
failing to call security_sk_alloc() is the first regression
introduced.

The second regression was the labeling issue. There was a call to
security_socket_post_create (from __sock_create) which was properly
setting the labels on both the socket and sock. This new patch broke
that as well. We don't expose an equivalent
security_sock_post_create() interface in the LSM currently, and until
we do, this can't be fixed. It's why I say it should be reverted.

I have a patch I'm testing right now which takes care of the first
part the way I like (and yes, I'm doing the allocation on the correct
number node). It basically looks like so:

+ for_each_possible_cpu(cpu) {
+ sock = &per_cpu(unicast_sock, cpu);
+ rc = security_sk_alloc(&sock->sk, PF_INET, GFP_KERNEL,
cpu_to_node(cpu));
+ if (rc)
+ return rc;
+ }

I'm going to work right now on exposing the equivalent struct sock LSM
interface so we can call that as well. But it's going to take me a
bit. Attached is the patch just to (hopefully untested) shut up the
panic.

-Eric

On Thu, Aug 9, 2012 at 10:50 AM, Eric Dumazet <[email protected]> wrote:
> From: Eric Dumazet <[email protected]>
>
> commit be9f4a44e7d41cee (ipv4: tcp: remove per net tcp_sock) added a
> selinux regression, reported and bisected by John Stultz
>
> selinux_ip_postroute_compat() expect to find a valid sk->sk_security
> pointer, but this field is NULL for unicast_sock
>
> Fix this by adding a new 'kernel' parameter to security_sk_alloc(),
> set to true if socket might already have a valid sk->sk_security
> pointer. ip_send_unicast_reply() uses a percpu fake socket, so the first
> call to security_sk_alloc() will populate sk->sk_security pointer,
> subsequent ones will reuse existing context.
>
> Reported-by: John Stultz <[email protected]>
> Bisected-by: John Stultz <[email protected]>
> Signed-off-by: Eric Dumazet <[email protected]>
> Cc: Paul Moore <[email protected]>
> Cc: Eric Paris <[email protected]>
> Cc: "Serge E. Hallyn" <[email protected]>
> ---
> include/linux/security.h | 6 +++---
> net/core/sock.c | 2 +-
> net/ipv4/ip_output.c | 4 +++-
> security/security.c | 4 ++--
> security/selinux/hooks.c | 5 ++++-
> security/smack/smack_lsm.c | 10 ++++++++--
> 6 files changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 4e5a73c..4d8e454 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1601,7 +1601,7 @@ struct security_operations {
> int (*socket_sock_rcv_skb) (struct sock *sk, struct sk_buff *skb);
> int (*socket_getpeersec_stream) (struct socket *sock, char __user *optval, int __user *optlen, unsigned len);
> int (*socket_getpeersec_dgram) (struct socket *sock, struct sk_buff *skb, u32 *secid);
> - int (*sk_alloc_security) (struct sock *sk, int family, gfp_t priority);
> + int (*sk_alloc_security) (struct sock *sk, int family, gfp_t priority, bool kernel);
> void (*sk_free_security) (struct sock *sk);
> void (*sk_clone_security) (const struct sock *sk, struct sock *newsk);
> void (*sk_getsecid) (struct sock *sk, u32 *secid);
> @@ -2539,7 +2539,7 @@ int security_sock_rcv_skb(struct sock *sk, struct sk_buff *skb);
> int security_socket_getpeersec_stream(struct socket *sock, char __user *optval,
> int __user *optlen, unsigned len);
> int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u32 *secid);
> -int security_sk_alloc(struct sock *sk, int family, gfp_t priority);
> +int security_sk_alloc(struct sock *sk, int family, gfp_t priority, bool kernel);
> void security_sk_free(struct sock *sk);
> void security_sk_clone(const struct sock *sk, struct sock *newsk);
> void security_sk_classify_flow(struct sock *sk, struct flowi *fl);
> @@ -2667,7 +2667,7 @@ static inline int security_socket_getpeersec_dgram(struct socket *sock, struct s
> return -ENOPROTOOPT;
> }
>
> -static inline int security_sk_alloc(struct sock *sk, int family, gfp_t priority)
> +static inline int security_sk_alloc(struct sock *sk, int family, gfp_t priority, bool kernel)
> {
> return 0;
> }
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 8f67ced..e00cadf 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1186,7 +1186,7 @@ static struct sock *sk_prot_alloc(struct proto *prot, gfp_t priority,
> if (sk != NULL) {
> kmemcheck_annotate_bitfield(sk, flags);
>
> - if (security_sk_alloc(sk, family, priority))
> + if (security_sk_alloc(sk, family, priority, false))
> goto out_free;
>
> if (!try_module_get(prot->owner))
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index 76dde25..b233d6e 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1524,6 +1524,8 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
> sk->sk_priority = skb->priority;
> sk->sk_protocol = ip_hdr(skb)->protocol;
> sk->sk_bound_dev_if = arg->bound_dev_if;
> + if (security_sk_alloc(sk, PF_INET, GFP_ATOMIC, true))
> + goto out;
> sock_net_set(sk, net);
> __skb_queue_head_init(&sk->sk_write_queue);
> sk->sk_sndbuf = sysctl_wmem_default;
> @@ -1539,7 +1541,7 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
> skb_set_queue_mapping(nskb, skb_get_queue_mapping(skb));
> ip_push_pending_frames(sk, &fl4);
> }
> -
> +out:
> put_cpu_var(unicast_sock);
>
> ip_rt_put(rt);
> diff --git a/security/security.c b/security/security.c
> index 860aeb3..23cf297 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1146,9 +1146,9 @@ int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u
> }
> EXPORT_SYMBOL(security_socket_getpeersec_dgram);
>
> -int security_sk_alloc(struct sock *sk, int family, gfp_t priority)
> +int security_sk_alloc(struct sock *sk, int family, gfp_t priority, bool kernel)
> {
> - return security_ops->sk_alloc_security(sk, family, priority);
> + return security_ops->sk_alloc_security(sk, family, priority, kernel);
> }
>
> void security_sk_free(struct sock *sk)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 6c77f63..ccd4374 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4289,10 +4289,13 @@ out:
> return 0;
> }
>
> -static int selinux_sk_alloc_security(struct sock *sk, int family, gfp_t priority)
> +static int selinux_sk_alloc_security(struct sock *sk, int family, gfp_t priority, bool kernel)
> {
> struct sk_security_struct *sksec;
>
> + if (kernel && sk->sk_security)
> + return 0;
> +
> sksec = kzalloc(sizeof(*sksec), priority);
> if (!sksec)
> return -ENOMEM;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 8221514..0b066d0 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1749,20 +1749,26 @@ static void smack_task_to_inode(struct task_struct *p, struct inode *inode)
> * @sk: the socket
> * @family: unused
> * @gfp_flags: memory allocation flags
> + * @kernel: true if we should check sk_security being already set
> *
> * Assign Smack pointers to current
> *
> * Returns 0 on success, -ENOMEM is there's no memory
> */
> -static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t gfp_flags)
> +static int smack_sk_alloc_security(struct sock *sk, int family, gfp_t gfp_flags, bool kernel)
> {
> - char *csp = smk_of_current();
> + char *csp;
> struct socket_smack *ssp;
>
> + if (kernel && sk->sk_security)
> + return 0;
> +
> ssp = kzalloc(sizeof(struct socket_smack), gfp_flags);
> if (ssp == NULL)
> return -ENOMEM;
>
> + csp = kernel ? smack_net_ambient : smk_of_current();
> +
> ssp->smk_in = csp;
> ssp->smk_out = csp;
> ssp->smk_packet = NULL;
>
>

Attachments:

tmp.patch (12.37 kB)

2012-08-09 20:20:40

[permalink] [raw]

Subject: Re: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock

On Thu, Aug 9, 2012 at 4:06 PM, Eric Paris <[email protected]> wrote:
> I'm going to work right now on exposing the equivalent struct sock LSM
> interface so we can call that as well. But it's going to take me a
> bit.

Before you go too far down this path, can you elaborate on what
exactly you mean by the above?

I'm asking because I'm not convinced the labeling, either the old way
or the new way, was 100% correct and I think we're going to need to
change things regardless. I'm just not sure what the right solution
is just yet.

--
paul moore
http://www.paul-moore.com

2012-08-09 21:29:15

[permalink] [raw]

Subject: Re: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock

On Thu, 2012-08-09 at 16:06 -0400, Eric Paris wrote:
> NAK.
>
> I personally think commit be9f4a44e7d41cee should be reverted until it
> is fixed. Let me explain what all I believe it broke and how.
>

Suggesting to revert this commit while we have known working fixes is a
bit of strange reaction.

I understand you are upset, but I believe we tried to fix it.

> Old callchain of the creation of the 'equivalent' socket previous to
> the patch in question just for reference:
>
> inet_ctl_sock_create
> sock_create_kern
> __sock_create
> pf->create (inet_create)
> sk_alloc
> sk_prot_alloc
> security_sk_alloc()
>
>
> This WAS working properly. All of it.

Nobody denies it. But acknowledge my patch uncovered a fundamental
issue.

What kind of 'security module' can decide to let RST packets being sent
or not, on a global scale ? (one socket for the whole machine)

smack_sk_alloc_security() uses smk_of_current() : What can be the
meaning of smk_of_current() in the context of 'kernel' sockets...

Your patch tries to maintain this status quo.

In fact I suggest the following one liner patch, unless you can really
demonstrate what can be the meaning of providing a fake socket for these
packets.

This mess only happened because ip_append_data()/ip_push_pending_frames()
are so complex and use an underlying socket.

But this socket should not be ever used outside of its scope.

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 76dde25..ec410e0 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1536,6 +1536,7 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
arg->csumoffset) = csum_fold(csum_add(nskb->csum,
arg->csum));
nskb->ip_summed = CHECKSUM_NONE;
+ skb_orphan(nskb);
skb_set_queue_mapping(nskb, skb_get_queue_mapping(skb));
ip_push_pending_frames(sk, &fl4);
}

2012-08-09 21:53:07

by Casey Schaufler

[permalink] [raw]

Subject: Re: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock

On 8/9/2012 2:29 PM, Eric Dumazet wrote:
> On Thu, 2012-08-09 at 16:06 -0400, Eric Paris wrote:
>> NAK.
>>
>> I personally think commit be9f4a44e7d41cee should be reverted until it
>> is fixed. Let me explain what all I believe it broke and how.
>>
> Suggesting to revert this commit while we have known working fixes is a
> bit of strange reaction.

A couple of potential short term workarounds have been identified,
but no one is happy with them for the long term. That does not
qualify as a "working fix" in engineering terms.

> I understand you are upset, but I believe we tried to fix it.
>
>> Old callchain of the creation of the 'equivalent' socket previous to
>> the patch in question just for reference:
>>
>> inet_ctl_sock_create
>> sock_create_kern
>> __sock_create
>> pf->create (inet_create)
>> sk_alloc
>> sk_prot_alloc
>> security_sk_alloc()
>>
>>
>> This WAS working properly. All of it.
> Nobody denies it. But acknowledge my patch uncovered a fundamental
> issue.
>
> What kind of 'security module' can decide to let RST packets being sent
> or not, on a global scale ? (one socket for the whole machine)

The short answer is "any security module that wants to".

And before we go any further, I'm a little surprised that
SELinux doesn't do this already.

>
> smack_sk_alloc_security() uses smk_of_current() : What can be the
> meaning of smk_of_current() in the context of 'kernel' sockets...

Yes, and all of it's callers - to date - have had an appropriate
value of current. It is using the API in the way it is supposed to.
It is assuming a properly formed socket. You want to give it a
cobbled together partial socket structure without task context.
Your predecessor did not have this problem.

>
> Your patch tries to maintain this status quo.
>
> In fact I suggest the following one liner patch, unless you can really
> demonstrate what can be the meaning of providing a fake socket for these
> packets.
>
> This mess only happened because ip_append_data()/ip_push_pending_frames()
> are so complex and use an underlying socket.
>
> But this socket should not be ever used outside of its scope.
>
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index 76dde25..ec410e0 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1536,6 +1536,7 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
> arg->csumoffset) = csum_fold(csum_add(nskb->csum,
> arg->csum));
> nskb->ip_summed = CHECKSUM_NONE;
> + skb_orphan(nskb);
> skb_set_queue_mapping(nskb, skb_get_queue_mapping(skb));
> ip_push_pending_frames(sk, &fl4);
> }
>
>
>

2012-08-09 22:05:28

[permalink] [raw]

Subject: Re: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock

On Thu, 2012-08-09 at 14:53 -0700, Casey Schaufler wrote:
> On 8/9/2012 2:29 PM, Eric Dumazet wrote:

> > smack_sk_alloc_security() uses smk_of_current() : What can be the
> > meaning of smk_of_current() in the context of 'kernel' sockets...
>
> Yes, and all of it's callers - to date - have had an appropriate
> value of current. It is using the API in the way it is supposed to.
> It is assuming a properly formed socket. You want to give it a
> cobbled together partial socket structure without task context.
> Your predecessor did not have this problem.

My predecessor ? You mean before the patch ?

tcp socket was preallocated by at kernel boot time.

What is the 'user' owning this socket ?

You guys focus on an implementation detail of TCP stack.
You should never use this fake socket.

I repeat : There are no true socket for these control packets.

If you want them, then you'll have to add fields in timewait socket.

I can decide to rewrite the whole thing just building a TCP packet on
its own, and send it without any fake socket.

Some guy 15 years ago tried to reuse some high level functions, able to
build super packets and use sophisticated tricks, while we only want so
send a 40 or 60 bytes packet.

2012-08-09 22:32:24

by Casey Schaufler

[permalink] [raw]

Subject: Re: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock

On 8/9/2012 3:05 PM, Eric Dumazet wrote:
> On Thu, 2012-08-09 at 14:53 -0700, Casey Schaufler wrote:
>> On 8/9/2012 2:29 PM, Eric Dumazet wrote:
>>> smack_sk_alloc_security() uses smk_of_current() : What can be the
>>> meaning of smk_of_current() in the context of 'kernel' sockets...
>> Yes, and all of it's callers - to date - have had an appropriate
>> value of current. It is using the API in the way it is supposed to.
>> It is assuming a properly formed socket. You want to give it a
>> cobbled together partial socket structure without task context.
>> Your predecessor did not have this problem.
> My predecessor ? You mean before the patch ?
>
> tcp socket was preallocated by at kernel boot time.
>
> What is the 'user' owning this socket ?
>
> You guys focus on an implementation detail of TCP stack.
> You should never use this fake socket.
>
> I repeat : There are no true socket for these control packets.
>
> If you want them, then you'll have to add fields in timewait socket.
>
> I can decide to rewrite the whole thing just building a TCP packet on
> its own, and send it without any fake socket.
>
> Some guy 15 years ago tried to reuse some high level functions, able to
> build super packets and use sophisticated tricks, while we only want so
> send a 40 or 60 bytes packet.

OK, fine. You have an optimization. I'm good with that. Just don't
expect that the entire software stack you are taking advantage of
is going to change to accommodate your special case.

2012-08-09 23:38:54

by David Miller

[permalink] [raw]

Subject: Re: [PATCH] ipv4: tcp: security_sk_alloc() needed for unicast_sock

From: Eric Dumazet <[email protected]>
Date: Thu, 09 Aug 2012 23:29:03 +0200

> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index 76dde25..ec410e0 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1536,6 +1536,7 @@ void ip_send_unicast_reply(struct net *net, struct sk_buff *skb, __be32 daddr,
> arg->csumoffset) = csum_fold(csum_add(nskb->csum,
> arg->csum));
> nskb->ip_summed = CHECKSUM_NONE;
> + skb_orphan(nskb);
> skb_set_queue_mapping(nskb, skb_get_queue_mapping(skb));
> ip_push_pending_frames(sk, &fl4);
> }
>

This is definitely the best fix, please submit this formally.

2012-08-09 23:56:15