[PATCH] Fix a potentially uninitialised variable in SELinux hooks

[PATCH] Fix a potentially uninitialised variable in SELinux hooks

[David Howells]

Fix a potentially uninitialised variable in SELinux hooks that's given a
pointer to the network address by selinux_parse_skb() passing a pointer back
through its argument list.  By restructuring selinux_parse_skb(), the compiler
can see that the error case need not set it as the caller will return
immediately.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/selinux/hooks.c |   42 ++++++++++++++++++++++++------------------
 1 files changed, 24 insertions(+), 18 deletions(-)


diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 85f74f6..3c87c59 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3513,38 +3513,44 @@ out:
 #endif /* IPV6 */
 
 static int selinux_parse_skb(struct sk_buff *skb, struct avc_audit_data *ad,
-			     char **addrp, int src, u8 *proto)
+			     char **_addrp, int src, u8 *proto)
 {
-	int ret = 0;
+	char *addrp;
+	int ret;
 
 	switch (ad->u.net.family) {
 	case PF_INET:
 		ret = selinux_parse_skb_ipv4(skb, ad, proto);
-		if (ret || !addrp)
-			break;
-		*addrp = (char *)(src ? &ad->u.net.v4info.saddr :
-					&ad->u.net.v4info.daddr);
-		break;
+		if (ret)
+			goto parse_error;
+		addrp = (char *)(src ? &ad->u.net.v4info.saddr :
+				       &ad->u.net.v4info.daddr);
+		goto okay;
 
 #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case PF_INET6:
 		ret = selinux_parse_skb_ipv6(skb, ad, proto);
-		if (ret || !addrp)
-			break;
-		*addrp = (char *)(src ? &ad->u.net.v6info.saddr :
-					&ad->u.net.v6info.daddr);
-		break;
+		if (ret)
+			goto parse_error;
+		addrp = (char *)(src ? &ad->u.net.v6info.saddr :
+				       &ad->u.net.v6info.daddr);
+		goto okay;
 #endif	/* IPV6 */
 	default:
-		break;
+		addrp = NULL;
+		goto okay;
 	}
 
-	if (unlikely(ret))
-		printk(KERN_WARNING
-		       "SELinux: failure in selinux_parse_skb(),"
-		       " unable to parse packet\n");
-
+parse_error:
+	printk(KERN_WARNING
+	       "SELinux: failure in selinux_parse_skb(),"
+	       " unable to parse packet\n");
 	return ret;
+
+okay:
+	if (_addrp)
+		*_addrp = addrp;
+	return 0;
 }
 
 /**


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] Fix a potentially uninitialised variable in SELinux hooks

[Paul Moore]

On Wednesday 09 July 2008 12:10:43 pm David Howells wrote:
> Fix a potentially uninitialised variable in SELinux hooks that's
> given a pointer to the network address by selinux_parse_skb() passing
> a pointer back through its argument list.  By restructuring
> selinux_parse_skb(), the compiler can see that the error case need
> not set it as the caller will return immediately.

Looking at all the callers of selinux_parse_skb() none of them actually 
pass a NULL value for addrp, which I believe is the only reason for 
conditionally setting the value of addrp in selinux_parse_skb().  I 
think a cleaner fix is to simply remove the conditional assignment so 
we have something that looks like this ...

	ret = selinux_parse_skb_ipv4(...);
	if (ret != 0)
		break;
	*addrp = (char *)(src ? ...);

I haven't checked but I'm pretty sure that would fix the problem you are 
seeing, yes?

> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
>  security/selinux/hooks.c |   42
> ++++++++++++++++++++++++------------------ 1 files changed, 24
> insertions(+), 18 deletions(-)
>
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 85f74f6..3c87c59 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3513,38 +3513,44 @@ out:
>  #endif /* IPV6 */
>
>  static int selinux_parse_skb(struct sk_buff *skb, struct
> avc_audit_data *ad, -			     char **addrp, int src, u8 *proto)
> +			     char **_addrp, int src, u8 *proto)
>  {
> -	int ret = 0;
> +	char *addrp;
> +	int ret;
>
>  	switch (ad->u.net.family) {
>  	case PF_INET:
>  		ret = selinux_parse_skb_ipv4(skb, ad, proto);
> -		if (ret || !addrp)
> -			break;
> -		*addrp = (char *)(src ? &ad->u.net.v4info.saddr :
> -					&ad->u.net.v4info.daddr);
> -		break;
> +		if (ret)
> +			goto parse_error;
> +		addrp = (char *)(src ? &ad->u.net.v4info.saddr :
> +				       &ad->u.net.v4info.daddr);
> +		goto okay;
>
>  #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>  	case PF_INET6:
>  		ret = selinux_parse_skb_ipv6(skb, ad, proto);
> -		if (ret || !addrp)
> -			break;
> -		*addrp = (char *)(src ? &ad->u.net.v6info.saddr :
> -					&ad->u.net.v6info.daddr);
> -		break;
> +		if (ret)
> +			goto parse_error;
> +		addrp = (char *)(src ? &ad->u.net.v6info.saddr :
> +				       &ad->u.net.v6info.daddr);
> +		goto okay;
>  #endif	/* IPV6 */
>  	default:
> -		break;
> +		addrp = NULL;
> +		goto okay;
>  	}
>
> -	if (unlikely(ret))
> -		printk(KERN_WARNING
> -		       "SELinux: failure in selinux_parse_skb(),"
> -		       " unable to parse packet\n");
> -
> +parse_error:
> +	printk(KERN_WARNING
> +	       "SELinux: failure in selinux_parse_skb(),"
> +	       " unable to parse packet\n");
>  	return ret;
> +
> +okay:
> +	if (_addrp)
> +		*_addrp = addrp;
> +	return 0;
>  }
>
>  /**
>
>
> --
> This message was distributed to subscribers of the selinux mailing
> list. If you no longer wish to subscribe, send mail to
> majordomo@tycho.nsa.gov with the words "unsubscribe selinux" without
> quotes as the message.



-- 
paul moore
linux @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] Fix a potentially uninitialised variable in SELinux hooks

[David Howells]

Paul Moore <paul.moore@hp.com> wrote:

> I haven't checked but I'm pretty sure that would fix the problem you are 
> seeing, yes?

Maybe.  The problem, I think, is that the error handling path diverges from
the main path and then converges again, and the compiler can't tell which way
it's going to go.

Besides, this way eliminates an if-statement.  I noticed that addrp was never
passed as NULL, but I was unsure as to whether this will always be the case.

David

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] Fix a potentially uninitialised variable in SELinux hooks

[Paul Moore]

On Wednesday 09 July 2008 6:39:02 pm David Howells wrote:
> Paul Moore <paul.moore@hp.com> wrote:
> > I haven't checked but I'm pretty sure that would fix the problem
> > you are seeing, yes?
>
> Maybe.  The problem, I think, is that the error handling path
> diverges from the main path and then converges again, and the
> compiler can't tell which way it's going to go.

Ah yes, my apologies.

> Besides, this way eliminates an if-statement.  I noticed that addrp
> was never passed as NULL, but I was unsure as to whether this will
> always be the case.

Well, it's a rather simple, local function so I'm not too concerned 
about what _might_ happen; I'm focusing more on what we have right now.  
For me personally, I'm not a huge fan of adding more variables and 
extra assignments to work around the problem; I'd prefer to fix it like 
this if we can ...

	ret = selinux_parse_skb_ipv4(...);
	if (ret != 0)
		addrp = (src ? ...);
	else
		addrp = NULL;

-- 
paul moore
linux @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] Fix a potentially uninitialised variable in SELinux hooks

[David Howells]

Paul Moore <paul.moore@hp.com> wrote:

> 	ret = selinux_parse_skb_ipv4(...);
> 	if (ret != 0)
> 		addrp = (src ? ...);
> 	else
> 		addrp = NULL;

I guess you mean "*addrp = ..." in that case, otherwise you haven't eliminated
anything.

Personally, I prefer to add extra variables if it makes things clearer, and I
prefer to use gotos for error handling.  It eliminates the else-statements that
you would otherwise introduce, and the goto-label can be used as documentation
of a sort too.  Furthermore, it moves the error handling clearly out of the
main route through the function.  Having said that, I should reorder my patch
to put the parse_error segment last - then I can ditch the okay label and use
break instead of goto.

It comes down to personal preference, I guess.

David

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] Fix a potentially uninitialised variable in SELinux hooks

[Paul Moore]

On Wednesday 09 July 2008 7:12:10 pm David Howells wrote:
> Paul Moore <paul.moore@hp.com> wrote:
> > 	ret = selinux_parse_skb_ipv4(...);
> > 	if (ret != 0)
> > 		addrp = (src ? ...);
> > 	else
> > 		addrp = NULL;
>
> I guess you mean "*addrp = ..." in that case, otherwise you haven't
> eliminated anything.

Yes, I did.  Too much pseudo in my pseudo-code I guess.

> Personally, I prefer to add extra variables if it makes things
> clearer, and I prefer to use gotos for error handling.  It eliminates
> the else-statements that you would otherwise introduce, and the
> goto-label can be used as documentation of a sort too.  Furthermore,
> it moves the error handling clearly out of the main route through the
> function.  Having said that, I should reorder my patch to put the
> parse_error segment last - then I can ditch the okay label and use
> break instead of goto.
>
> It comes down to personal preference, I guess.

Doesn't it always :)

I don't think it makes a huge difference either way, and since you are 
the one fixing the problem I'll let you decide.

-- 
paul moore
linux @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.