Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

On Thu, Aug 9, 2012 at 10:27 AM, Eric Dumazet  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
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Dumazet]

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.



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

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
www.paul-moore.com

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

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
www.paul-moore.com

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Dumazet]

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.



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

On Thu, Aug 9, 2012 at 10:27 AM, Eric Dumazet eric.duma...@gmail.com 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
www.paul-moore.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Casey Schaufler]

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 

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Dumazet]

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 

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Paris]

On Wed, Aug 8, 2012 at 5:03 PM, Paul Moore  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...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

On Wednesday, August 08, 2012 04:51:56 PM Eric Paris wrote:
> On Wed, Aug 8, 2012 at 4:35 PM, Paul Moore  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
www.paul-moore.com

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Paris]

On Wed, Aug 8, 2012 at 4:35 PM, Paul Moore  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...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

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
www.paul-moore.com

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

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
www.paul-moore.com

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Dumazet]

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_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, );
}
-
+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:
   

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Dumazet]

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,
};


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Dumazet]

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 !



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Dumazet]

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 

John, could you confirm this fixes the problem ?


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Paris]

On Wed, Aug 8, 2012 at 3:38 PM, Eric Dumazet  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_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, );
> }
> -
> +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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

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 
> > > Date:   Thu Jul 19 07:34:03 2012 +
> > > 
> > >  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 

> 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_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, );
>   }
> -
> +out:
>   put_cpu_var(unicast_sock);
> 
>   ip_rt_put(rt);

-- 
paul moore
www.paul-moore.com

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[John Stultz]

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_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, );
}
-
+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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Dumazet]

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 
> > Date:   Thu Jul 19 07:34:03 2012 +
> > 
> >  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_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, );
}
-
+out:
put_cpu_var(unicast_sock);
 
ip_rt_put(rt);


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Dumazet]

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 (p...@paul-moore.com):
> >>> On Tue, Aug 7, 2012 at 5:58 PM, John Stultz  
> >>> wrote:
>  On 08/07/2012 02:50 PM, Paul Moore wrote:
> > On Tue, Aug 7, 2012 at 2:12 PM, John Stultz 
> > 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, , 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 
> Date:   Thu Jul 19 07:34:03 2012 +
> 
>  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().



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

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 
> Date:   Thu Jul 19 07:34:03 2012 +
> 
>  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
www.paul-moore.com

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[John Stultz]

On 08/07/2012 03:37 PM, John Stultz wrote:

On 08/07/2012 03:17 PM, Serge E. Hallyn wrote:

Quoting Paul Moore (p...@paul-moore.com):
On Tue, Aug 7, 2012 at 5:58 PM, John Stultz  
wrote:

On 08/07/2012 02:50 PM, Paul Moore wrote:

On Tue, Aug 7, 2012 at 2:12 PM, John Stultz 
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, , 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 
Date:   Thu Jul 19 07:34:03 2012 +

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[John Johansen]

On 08/07/2012 03:17 PM, Serge E. Hallyn wrote:
> Quoting Paul Moore (p...@paul-moore.com):
>> On Tue, Aug 7, 2012 at 5:58 PM, John Stultz  wrote:
>>> On 08/07/2012 02:50 PM, Paul Moore wrote:

 On Tue, Aug 7, 2012 at 2:12 PM, John Stultz 
 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, , 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[John Johansen]

On 08/07/2012 03:17 PM, Serge E. Hallyn wrote:
 Quoting Paul Moore (p...@paul-moore.com):
 On Tue, Aug 7, 2012 at 5:58 PM, John Stultz john.stu...@linaro.org wrote:
 On 08/07/2012 02:50 PM, Paul Moore wrote:

 On Tue, Aug 7, 2012 at 2:12 PM, John Stultz john.stu...@linaro.org
 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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[John Stultz]

On 08/07/2012 03:37 PM, John Stultz wrote:

On 08/07/2012 03:17 PM, Serge E. Hallyn wrote:

Quoting Paul Moore (p...@paul-moore.com):
On Tue, Aug 7, 2012 at 5:58 PM, John Stultz john.stu...@linaro.org 
wrote:

On 08/07/2012 02:50 PM, Paul Moore wrote:

On Tue, Aug 7, 2012 at 2:12 PM, John Stultz john.stu...@linaro.org
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 eduma...@google.com
Date:   Thu Jul 19 07:34:03 2012 +

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


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

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 eduma...@google.com
 Date:   Thu Jul 19 07:34:03 2012 +
 
  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
www.paul-moore.com

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Dumazet]

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 (p...@paul-moore.com):
  On Tue, Aug 7, 2012 at 5:58 PM, John Stultz john.stu...@linaro.org 
  wrote:
  On 08/07/2012 02:50 PM, Paul Moore wrote:
  On Tue, Aug 7, 2012 at 2:12 PM, John Stultz john.stu...@linaro.org
  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 eduma...@google.com
 Date:   Thu Jul 19 07:34:03 2012 +
 
  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().



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Dumazet]

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 eduma...@google.com
  Date:   Thu Jul 19 07:34:03 2012 +
  
   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);


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[John Stultz]

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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

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 eduma...@google.com
   Date:   Thu Jul 19 07:34:03 2012 +
   
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 p...@paul-moore.com

 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
www.paul-moore.com

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Paris]

On Wed, Aug 8, 2012 at 3:38 PM, Eric Dumazet eric.duma...@gmail.com 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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Dumazet]

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 p...@paul-moore.com

John, could you confirm this fixes the problem ?


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Dumazet]

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 !



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Dumazet]

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,
};


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Dumazet]

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;
 }
 

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

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
www.paul-moore.com

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

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
www.paul-moore.com

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Paris]

On Wed, Aug 8, 2012 at 4:35 PM, Paul Moore p...@paul-moore.com 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...
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

On Wednesday, August 08, 2012 04:51:56 PM Eric Paris wrote:
 On Wed, Aug 8, 2012 at 4:35 PM, Paul Moore p...@paul-moore.com 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
www.paul-moore.com

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Paris]

On Wed, Aug 8, 2012 at 5:03 PM, Paul Moore p...@paul-moore.com 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...
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Eric Dumazet]

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, 

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Casey Schaufler]

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)
  {
   

Re: NULL pointer dereference in selinux_ip_postroute_compat

[John Stultz]

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  
wrote:

On 08/07/2012 02:50 PM, Paul Moore wrote:

On Tue, Aug 7, 2012 at 2:12 PM, John Stultz 
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, , 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[John Stultz]

On 08/07/2012 03:17 PM, Serge E. Hallyn wrote:

Quoting Paul Moore (p...@paul-moore.com):

On Tue, Aug 7, 2012 at 5:58 PM, John Stultz  wrote:

On 08/07/2012 02:50 PM, Paul Moore wrote:

On Tue, Aug 7, 2012 at 2:12 PM, John Stultz 
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, , 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[John Stultz]

On 08/07/2012 03:01 PM, Paul Moore wrote:

On Tue, Aug 7, 2012 at 5:58 PM, John Stultz  wrote:

On 08/07/2012 02:50 PM, Paul Moore wrote:

On Tue, Aug 7, 2012 at 2:12 PM, John Stultz 
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, , 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

On Tuesday, August 07, 2012 10:17:32 PM Serge E. Hallyn wrote:
> Quoting Paul Moore (p...@paul-moore.com):
> > On Tue, Aug 7, 2012 at 5:58 PM, John Stultz  
wrote:
> > > On 08/07/2012 02:50 PM, Paul Moore wrote:
> > >> On Tue, Aug 7, 2012 at 2:12 PM, John Stultz 
> > >> 
> > >> 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, , 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
www.paul-moore.com

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Serge E. Hallyn]

Quoting Paul Moore (p...@paul-moore.com):
> On Tue, Aug 7, 2012 at 5:58 PM, John Stultz  wrote:
> > On 08/07/2012 02:50 PM, Paul Moore wrote:
> >>
> >> On Tue, Aug 7, 2012 at 2:12 PM, John Stultz 
> >> 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, , 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

On Tue, Aug 7, 2012 at 5:58 PM, John Stultz  wrote:
> On 08/07/2012 02:50 PM, Paul Moore wrote:
>>
>> On Tue, Aug 7, 2012 at 2:12 PM, John Stultz 
>> 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, , 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
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[John Stultz]

On 08/07/2012 02:50 PM, Paul Moore wrote:

On Tue, Aug 7, 2012 at 2:12 PM, John Stultz  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, , 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

On Tue, Aug 7, 2012 at 2:12 PM, John Stultz  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, , 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
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

On Tue, Aug 7, 2012 at 2:12 PM, John Stultz john.stu...@linaro.org 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
www.paul-moore.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[John Stultz]

On 08/07/2012 02:50 PM, Paul Moore wrote:

On Tue, Aug 7, 2012 at 2:12 PM, John Stultz john.stu...@linaro.org 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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

On Tue, Aug 7, 2012 at 5:58 PM, John Stultz john.stu...@linaro.org wrote:
 On 08/07/2012 02:50 PM, Paul Moore wrote:

 On Tue, Aug 7, 2012 at 2:12 PM, John Stultz john.stu...@linaro.org
 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
www.paul-moore.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Serge E. Hallyn]

Quoting Paul Moore (p...@paul-moore.com):
 On Tue, Aug 7, 2012 at 5:58 PM, John Stultz john.stu...@linaro.org wrote:
  On 08/07/2012 02:50 PM, Paul Moore wrote:
 
  On Tue, Aug 7, 2012 at 2:12 PM, John Stultz john.stu...@linaro.org
  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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[Paul Moore]

On Tuesday, August 07, 2012 10:17:32 PM Serge E. Hallyn wrote:
 Quoting Paul Moore (p...@paul-moore.com):
  On Tue, Aug 7, 2012 at 5:58 PM, John Stultz john.stu...@linaro.org 
wrote:
   On 08/07/2012 02:50 PM, Paul Moore wrote:
   On Tue, Aug 7, 2012 at 2:12 PM, John Stultz john.stu...@linaro.org
   
   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
www.paul-moore.com

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[John Stultz]

On 08/07/2012 03:01 PM, Paul Moore wrote:

On Tue, Aug 7, 2012 at 5:58 PM, John Stultz john.stu...@linaro.org wrote:

On 08/07/2012 02:50 PM, Paul Moore wrote:

On Tue, Aug 7, 2012 at 2:12 PM, John Stultz john.stu...@linaro.org
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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[John Stultz]

On 08/07/2012 03:17 PM, Serge E. Hallyn wrote:

Quoting Paul Moore (p...@paul-moore.com):

On Tue, Aug 7, 2012 at 5:58 PM, John Stultz john.stu...@linaro.org wrote:

On 08/07/2012 02:50 PM, Paul Moore wrote:

On Tue, Aug 7, 2012 at 2:12 PM, John Stultz john.stu...@linaro.org
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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: NULL pointer dereference in selinux_ip_postroute_compat

[John Stultz]

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 john.stu...@linaro.org 
wrote:

On 08/07/2012 02:50 PM, Paul Moore wrote:

On Tue, Aug 7, 2012 at 2:12 PM, John Stultz john.stu...@linaro.org
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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/