| Message ID | 20190322141414.496017-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | selinux: avoid uninitialized variable warning | expand |
On Fri, Mar 22, 2019 at 03:14:10PM +0100, Arnd Bergmann wrote: > clang correctly points out a code path that would lead > to an uninitialized variable use: > > security/selinux/netlabel.c:310:6: error: variable 'addr' is used uninitialized whenever 'if' condition is false > [-Werror,-Wsometimes-uninitialized] > if (ip_hdr(skb)->version == 4) { > ^~~~~~~~~~~~~~~~~~~~~~~~~ > security/selinux/netlabel.c:322:40: note: uninitialized use occurs here > rc = netlbl_conn_setattr(ep->base.sk, addr, &secattr); > ^~~~ > security/selinux/netlabel.c:310:2: note: remove the 'if' if its condition is always true > if (ip_hdr(skb)->version == 4) { > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > security/selinux/netlabel.c:291:23: note: initialize the variable 'addr' to silence this warning > struct sockaddr *addr; > ^ > = NULL > 1 error generated. > > This is probably harmless since we should not see ipv6 packets > of CONFIG_IPV6 is disabled, but it's better to rearrange the code > so this cannot happen. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Reviewed-by: Nathan Chancellor <natechancellor@gmail.com> > --- > security/selinux/netlabel.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c > index 186e727b737b..d0e549d4f486 100644 > --- a/security/selinux/netlabel.c > +++ b/security/selinux/netlabel.c > @@ -288,7 +288,6 @@ int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep, > int rc; > struct netlbl_lsm_secattr secattr; > struct sk_security_struct *sksec = ep->base.sk->sk_security; > - struct sockaddr *addr; > struct sockaddr_in addr4; > #if IS_ENABLED(CONFIG_IPV6) > struct sockaddr_in6 addr6; > @@ -310,16 +309,15 @@ int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep, > if (ip_hdr(skb)->version == 4) { > addr4.sin_family = AF_INET; > addr4.sin_addr.s_addr = ip_hdr(skb)->saddr; > - addr = (struct sockaddr *)&addr4; > + rc = netlbl_conn_setattr(ep->base.sk, (void*)&addr4, &secattr); > #if IS_ENABLED(CONFIG_IPV6) > } else { > addr6.sin6_family = AF_INET6; > addr6.sin6_addr = ipv6_hdr(skb)->saddr; > - addr = (struct sockaddr *)&addr6; > + rc = netlbl_conn_setattr(ep->base.sk, (void*)&addr6, &secattr); > #endif > } > > - rc = netlbl_conn_setattr(ep->base.sk, addr, &secattr); > if (rc == 0) > sksec->nlbl_state = NLBL_LABELED; > > -- > 2.20.0 >
On Fri, Mar 22, 2019 at 10:14 AM Arnd Bergmann <arnd@arndb.de> wrote: > clang correctly points out a code path that would lead > to an uninitialized variable use: > > security/selinux/netlabel.c:310:6: error: variable 'addr' is used uninitialized whenever 'if' condition is false > [-Werror,-Wsometimes-uninitialized] > if (ip_hdr(skb)->version == 4) { > ^~~~~~~~~~~~~~~~~~~~~~~~~ > security/selinux/netlabel.c:322:40: note: uninitialized use occurs here > rc = netlbl_conn_setattr(ep->base.sk, addr, &secattr); > ^~~~ > security/selinux/netlabel.c:310:2: note: remove the 'if' if its condition is always true > if (ip_hdr(skb)->version == 4) { > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > security/selinux/netlabel.c:291:23: note: initialize the variable 'addr' to silence this warning > struct sockaddr *addr; > ^ > = NULL > 1 error generated. > > This is probably harmless since we should not see ipv6 packets > of CONFIG_IPV6 is disabled, but it's better to rearrange the code > so this cannot happen. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > security/selinux/netlabel.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) Hi Arnd, Thanks for pointing this out and providing a fix. I think you're right in that the should be pretty harmless, but I also agree that we should fix it; some thoughts on the patch below ... > diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c > index 186e727b737b..d0e549d4f486 100644 > --- a/security/selinux/netlabel.c > +++ b/security/selinux/netlabel.c > @@ -288,7 +288,6 @@ int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep, > int rc; > struct netlbl_lsm_secattr secattr; > struct sk_security_struct *sksec = ep->base.sk->sk_security; > - struct sockaddr *addr; > struct sockaddr_in addr4; > #if IS_ENABLED(CONFIG_IPV6) > struct sockaddr_in6 addr6; > @@ -310,16 +309,15 @@ int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep, > if (ip_hdr(skb)->version == 4) { > addr4.sin_family = AF_INET; > addr4.sin_addr.s_addr = ip_hdr(skb)->saddr; > - addr = (struct sockaddr *)&addr4; > + rc = netlbl_conn_setattr(ep->base.sk, (void*)&addr4, &secattr); > #if IS_ENABLED(CONFIG_IPV6) > } else { > addr6.sin6_family = AF_INET6; > addr6.sin6_addr = ipv6_hdr(skb)->saddr; > - addr = (struct sockaddr *)&addr6; > + rc = netlbl_conn_setattr(ep->base.sk, (void*)&addr6, &secattr); > #endif While we are hardening the code a bit, I'm thinking we should probably refactor this if-else a bit, some pseudo code for example: if (ip_hdr == 4) { rc = netlbl_conn_setattr(); #if CONFIG_IPV6 } else if (ip_hdr == 6) { rc = netlbl_conn_setattr(); #endif } else { rc = -EAFNOSUPPORT; } Thoughts? > } > > - rc = netlbl_conn_setattr(ep->base.sk, addr, &secattr); > if (rc == 0) > sksec->nlbl_state = NLBL_LABELED; > > -- > 2.20.0 >
On Fri, Mar 22, 2019 at 9:15 PM Paul Moore <paul@paul-moore.com> wrote: > On Fri, Mar 22, 2019 at 10:14 AM Arnd Bergmann <arnd@arndb.de> wrote: > Hi Arnd, > > Thanks for pointing this out and providing a fix. I think you're > right in that the should be pretty harmless, but I also agree that we > should fix it; some thoughts on the patch below ... > > > diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c > > index 186e727b737b..d0e549d4f486 100644 > > --- a/security/selinux/netlabel.c > > +++ b/security/selinux/netlabel.c > > @@ -288,7 +288,6 @@ int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep, > > int rc; > > struct netlbl_lsm_secattr secattr; > > struct sk_security_struct *sksec = ep->base.sk->sk_security; > > - struct sockaddr *addr; > > struct sockaddr_in addr4; > > #if IS_ENABLED(CONFIG_IPV6) > > struct sockaddr_in6 addr6; > > @@ -310,16 +309,15 @@ int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep, > > if (ip_hdr(skb)->version == 4) { > > addr4.sin_family = AF_INET; > > addr4.sin_addr.s_addr = ip_hdr(skb)->saddr; > > - addr = (struct sockaddr *)&addr4; > > + rc = netlbl_conn_setattr(ep->base.sk, (void*)&addr4, &secattr); > > #if IS_ENABLED(CONFIG_IPV6) > > } else { > > addr6.sin6_family = AF_INET6; > > addr6.sin6_addr = ipv6_hdr(skb)->saddr; > > - addr = (struct sockaddr *)&addr6; > > + rc = netlbl_conn_setattr(ep->base.sk, (void*)&addr6, &secattr); > > #endif > > While we are hardening the code a bit, I'm thinking we should probably > refactor this if-else a bit, some pseudo code for example: > > if (ip_hdr == 4) { > rc = netlbl_conn_setattr(); > #if CONFIG_IPV6 > } else if (ip_hdr == 6) { > rc = netlbl_conn_setattr(); > #endif > } else { > rc = -EAFNOSUPPORT; > } > > Thoughts? > Seems fine. We could go a step further and use IS_ENABLED() as C code here to get rid of the two #ifdef checks as well, like if (ip_hdr(skb)->version == 4 ) { addr4.sin_family = AF_INET; addr4.sin_addr.s_addr = ip_hdr(skb)->saddr; rc = netlbl_conn_setattr(ep->base.sk, &addr4, &secattr); } else if (IS_ENABLED(CONFIG_IPV6) && ip_hdr(skb)->version == 6) { addr6.sin6_family = AF_INET6; addr6.sin6_addr = ipv6_hdr(skb)->saddr; rc = netlbl_conn_setattr(ep->base.sk, &addr6, &secattr); } else { rc = -EAFNOSUPPORT; } Arnd
On Fri, Mar 22, 2019 at 4:35 PM Arnd Bergmann <arnd@arndb.de> wrote: > On Fri, Mar 22, 2019 at 9:15 PM Paul Moore <paul@paul-moore.com> wrote: > > On Fri, Mar 22, 2019 at 10:14 AM Arnd Bergmann <arnd@arndb.de> wrote: > > > Hi Arnd, > > > > Thanks for pointing this out and providing a fix. I think you're > > right in that the should be pretty harmless, but I also agree that we > > should fix it; some thoughts on the patch below ... > > > > > diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c > > > index 186e727b737b..d0e549d4f486 100644 > > > --- a/security/selinux/netlabel.c > > > +++ b/security/selinux/netlabel.c > > > @@ -288,7 +288,6 @@ int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep, > > > int rc; > > > struct netlbl_lsm_secattr secattr; > > > struct sk_security_struct *sksec = ep->base.sk->sk_security; > > > - struct sockaddr *addr; > > > struct sockaddr_in addr4; > > > #if IS_ENABLED(CONFIG_IPV6) > > > struct sockaddr_in6 addr6; > > > @@ -310,16 +309,15 @@ int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep, > > > if (ip_hdr(skb)->version == 4) { > > > addr4.sin_family = AF_INET; > > > addr4.sin_addr.s_addr = ip_hdr(skb)->saddr; > > > - addr = (struct sockaddr *)&addr4; > > > + rc = netlbl_conn_setattr(ep->base.sk, (void*)&addr4, &secattr); > > > #if IS_ENABLED(CONFIG_IPV6) > > > } else { > > > addr6.sin6_family = AF_INET6; > > > addr6.sin6_addr = ipv6_hdr(skb)->saddr; > > > - addr = (struct sockaddr *)&addr6; > > > + rc = netlbl_conn_setattr(ep->base.sk, (void*)&addr6, &secattr); > > > #endif > > > > While we are hardening the code a bit, I'm thinking we should probably > > refactor this if-else a bit, some pseudo code for example: > > > > if (ip_hdr == 4) { > > rc = netlbl_conn_setattr(); > > #if CONFIG_IPV6 > > } else if (ip_hdr == 6) { > > rc = netlbl_conn_setattr(); > > #endif > > } else { > > rc = -EAFNOSUPPORT; > > } > > > > Thoughts? > > > > Seems fine. We could go a step further and use IS_ENABLED() > as C code here to get rid of the two #ifdef checks as well, like > > if (ip_hdr(skb)->version == 4 ) { > addr4.sin_family = AF_INET; > addr4.sin_addr.s_addr = ip_hdr(skb)->saddr; > rc = netlbl_conn_setattr(ep->base.sk, &addr4, &secattr); > } else if (IS_ENABLED(CONFIG_IPV6) && ip_hdr(skb)->version == 6) { > addr6.sin6_family = AF_INET6; > addr6.sin6_addr = ipv6_hdr(skb)->saddr; > rc = netlbl_conn_setattr(ep->base.sk, &addr6, &secattr); > } else { > rc = -EAFNOSUPPORT; > } Looks good to me. Can you send a revised patch?
diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c index 186e727b737b..d0e549d4f486 100644 --- a/security/selinux/netlabel.c +++ b/security/selinux/netlabel.c @@ -288,7 +288,6 @@ int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep, int rc; struct netlbl_lsm_secattr secattr; struct sk_security_struct *sksec = ep->base.sk->sk_security; - struct sockaddr *addr; struct sockaddr_in addr4; #if IS_ENABLED(CONFIG_IPV6) struct sockaddr_in6 addr6; @@ -310,16 +309,15 @@ int selinux_netlbl_sctp_assoc_request(struct sctp_endpoint *ep, if (ip_hdr(skb)->version == 4) { addr4.sin_family = AF_INET; addr4.sin_addr.s_addr = ip_hdr(skb)->saddr; - addr = (struct sockaddr *)&addr4; + rc = netlbl_conn_setattr(ep->base.sk, (void*)&addr4, &secattr); #if IS_ENABLED(CONFIG_IPV6) } else { addr6.sin6_family = AF_INET6; addr6.sin6_addr = ipv6_hdr(skb)->saddr; - addr = (struct sockaddr *)&addr6; + rc = netlbl_conn_setattr(ep->base.sk, (void*)&addr6, &secattr); #endif } - rc = netlbl_conn_setattr(ep->base.sk, addr, &secattr); if (rc == 0) sksec->nlbl_state = NLBL_LABELED;
clang correctly points out a code path that would lead to an uninitialized variable use: security/selinux/netlabel.c:310:6: error: variable 'addr' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized] if (ip_hdr(skb)->version == 4) { ^~~~~~~~~~~~~~~~~~~~~~~~~ security/selinux/netlabel.c:322:40: note: uninitialized use occurs here rc = netlbl_conn_setattr(ep->base.sk, addr, &secattr); ^~~~ security/selinux/netlabel.c:310:2: note: remove the 'if' if its condition is always true if (ip_hdr(skb)->version == 4) { ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ security/selinux/netlabel.c:291:23: note: initialize the variable 'addr' to silence this warning struct sockaddr *addr; ^ = NULL 1 error generated. This is probably harmless since we should not see ipv6 packets of CONFIG_IPV6 is disabled, but it's better to rearrange the code so this cannot happen. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- security/selinux/netlabel.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)