Patchwork netfilter: potential NULL dereference in get_inner_hdr()

login
register
mail settings
Submitter Dan Carpenter
Date May 12, 2012, 11 a.m.
Message ID <20120512110003.GB19472@elgon.mountain>
Download mbox | patch
Permalink /patch/158730/
State Accepted
Headers show

Comments

Dan Carpenter - May 12, 2012, 11 a.m.
There is a typo in the error checking and "&&" was used instead of "||".
If skb_header_pointer() returns NULL then it leads to a NULL
dereference.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Btw, this is new code and Sparse complains about endian bugs.

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans Schillstrom - May 14, 2012, 7:36 a.m.
On Saturday 12 May 2012 13:00:03 Dan Carpenter wrote:
> There is a typo in the error checking and "&&" was used instead of "||".
> If skb_header_pointer() returns NULL then it leads to a NULL
> dereference.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Ack-by: Hans Schillstrom <hans.schillstrom@ericsson.com>
 
> ---
> Btw, this is new code and Sparse complains about endian bugs.

Can you give me some hints here, arch , compiler version etc.
I guess it was input to hmark_addr_mask() that complains ?

> 
> diff --git a/net/netfilter/xt_HMARK.c b/net/netfilter/xt_HMARK.c
> index 32fbd73..5817d03 100644
> --- a/net/netfilter/xt_HMARK.c
> +++ b/net/netfilter/xt_HMARK.c
> @@ -223,7 +223,7 @@ static int get_inner_hdr(const struct sk_buff *skb, int iphsz, int *nhoff)
>  
>  	/* Not enough header? */
>  	icmph = skb_header_pointer(skb, *nhoff + iphsz, sizeof(_ih), &_ih);
> -	if (icmph == NULL && icmph->type > NR_ICMP_TYPES)
> +	if (icmph == NULL || icmph->type > NR_ICMP_TYPES)
>  		return 0;
>  
>  	/* Error message? */
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
David Miller - May 14, 2012, 7:38 a.m.
From: Hans Schillstrom <hans.schillstrom@ericsson.com>
Date: Mon, 14 May 2012 09:36:55 +0200

> On Saturday 12 May 2012 13:00:03 Dan Carpenter wrote:
>> There is a typo in the error checking and "&&" was used instead of "||".
>> If skb_header_pointer() returns NULL then it leads to a NULL
>> dereference.
>> 
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> Ack-by: Hans Schillstrom <hans.schillstrom@ericsson.com>
>  
>> ---
>> Btw, this is new code and Sparse complains about endian bugs.
> 
> Can you give me some hints here, arch , compiler version etc.
> I guess it was input to hmark_addr_mask() that complains ?

He said what he's using, "sparse", the semantic parser, which
is largely arch agnostic.

I guarantee you will see the warnings if you run it on your
system on this code.


--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - May 14, 2012, 7:53 a.m.
On Mon, 2012-05-14 at 03:38 -0400, David Miller wrote:

> He said what he's using, "sparse", the semantic parser, which
> is largely arch agnostic.
> 
> I guarantee you will see the warnings if you run it on your
> system on this code.

Some more info :

http://lwn.net/Articles/205624/

make C=2 CF=-D__CHECK_ENDIAN__ net/netfilter/xt_HMARK.o
  CHECK   net/netfilter/xt_HMARK.c
net/netfilter/xt_HMARK.c:87:21: warning: incorrect type in assignment (different base types)
net/netfilter/xt_HMARK.c:87:21:    expected unsigned short [unsigned] [usertype] src
net/netfilter/xt_HMARK.c:87:21:    got restricted __be16 [usertype] all
net/netfilter/xt_HMARK.c:88:21: warning: incorrect type in assignment (different base types)
net/netfilter/xt_HMARK.c:88:21:    expected unsigned short [unsigned] [usertype] dst
net/netfilter/xt_HMARK.c:88:21:    got restricted __be16 [usertype] all
net/netfilter/xt_HMARK.c:170:27: warning: incorrect type in argument 2 (different signedness)
net/netfilter/xt_HMARK.c:170:27:    expected int *offset
net/netfilter/xt_HMARK.c:170:27:    got unsigned int *<noident>
net/netfilter/xt_HMARK.c:181:28: warning: incorrect type in argument 1 (different base types)
net/netfilter/xt_HMARK.c:181:28:    expected unsigned int const [usertype] *addr32
net/netfilter/xt_HMARK.c:181:28:    got restricted __be32 *<noident>
net/netfilter/xt_HMARK.c:182:28: warning: incorrect type in argument 1 (different base types)
net/netfilter/xt_HMARK.c:182:28:    expected unsigned int const [usertype] *addr32
net/netfilter/xt_HMARK.c:182:28:    got restricted __be32 *<noident>
net/netfilter/xt_HMARK.c:261:9: warning: invalid assignment: &=
net/netfilter/xt_HMARK.c:261:9:    left side has type unsigned int
net/netfilter/xt_HMARK.c:261:9:    right side has type restricted __be32
net/netfilter/xt_HMARK.c:262:9: warning: invalid assignment: &=
net/netfilter/xt_HMARK.c:262:9:    left side has type unsigned int
net/netfilter/xt_HMARK.c:262:9:    right side has type restricted __be32




--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans Schillstrom - May 14, 2012, 8 a.m.
On Monday 14 May 2012 09:53:15 Eric Dumazet wrote:
> On Mon, 2012-05-14 at 03:38 -0400, David Miller wrote:
> 
> > He said what he's using, "sparse", the semantic parser, which
> > is largely arch agnostic.
> > 
> > I guarantee you will see the warnings if you run it on your
> > system on this code.
> 
> Some more info :
> 
> http://lwn.net/Articles/205624/
> 
> make C=2 CF=-D__CHECK_ENDIAN__ net/netfilter/xt_HMARK.o
>   CHECK   net/netfilter/xt_HMARK.c
> net/netfilter/xt_HMARK.c:87:21: warning: incorrect type in assignment (different base types)
> net/netfilter/xt_HMARK.c:87:21:    expected unsigned short [unsigned] [usertype] src
> net/netfilter/xt_HMARK.c:87:21:    got restricted __be16 [usertype] all
> net/netfilter/xt_HMARK.c:88:21: warning: incorrect type in assignment (different base types)
> net/netfilter/xt_HMARK.c:88:21:    expected unsigned short [unsigned] [usertype] dst
> net/netfilter/xt_HMARK.c:88:21:    got restricted __be16 [usertype] all
> net/netfilter/xt_HMARK.c:170:27: warning: incorrect type in argument 2 (different signedness)
> net/netfilter/xt_HMARK.c:170:27:    expected int *offset
> net/netfilter/xt_HMARK.c:170:27:    got unsigned int *<noident>
> net/netfilter/xt_HMARK.c:181:28: warning: incorrect type in argument 1 (different base types)
> net/netfilter/xt_HMARK.c:181:28:    expected unsigned int const [usertype] *addr32
> net/netfilter/xt_HMARK.c:181:28:    got restricted __be32 *<noident>
> net/netfilter/xt_HMARK.c:182:28: warning: incorrect type in argument 1 (different base types)
> net/netfilter/xt_HMARK.c:182:28:    expected unsigned int const [usertype] *addr32
> net/netfilter/xt_HMARK.c:182:28:    got restricted __be32 *<noident>
> net/netfilter/xt_HMARK.c:261:9: warning: invalid assignment: &=
> net/netfilter/xt_HMARK.c:261:9:    left side has type unsigned int
> net/netfilter/xt_HMARK.c:261:9:    right side has type restricted __be32
> net/netfilter/xt_HMARK.c:262:9: warning: invalid assignment: &=
> net/netfilter/xt_HMARK.c:262:9:    left side has type unsigned int
> net/netfilter/xt_HMARK.c:262:9:    right side has type restricted __be32
> 

Thanks 
I'll try to fix the warnings
Pablo Neira - May 14, 2012, 8:22 a.m.
On Mon, May 14, 2012 at 10:00:00AM +0200, Hans Schillstrom wrote:
> > 
> > http://lwn.net/Articles/205624/
> > 
> > make C=2 CF=-D__CHECK_ENDIAN__ net/netfilter/xt_HMARK.o
> >   CHECK   net/netfilter/xt_HMARK.c
> > net/netfilter/xt_HMARK.c:87:21: warning: incorrect type in assignment (different base types)
> > net/netfilter/xt_HMARK.c:87:21:    expected unsigned short [unsigned] [usertype] src
> > net/netfilter/xt_HMARK.c:87:21:    got restricted __be16 [usertype] all
[...]
> 
> Thanks 
> I'll try to fix the warnings

Ok, wait for that patch, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira - May 14, 2012, 8:24 a.m.
On Sat, May 12, 2012 at 02:00:03PM +0300, Dan Carpenter wrote:
> There is a typo in the error checking and "&&" was used instead of "||".
> If skb_header_pointer() returns NULL then it leads to a NULL
> dereference.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied, thanks Dan.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Carpenter - May 14, 2012, 8:39 a.m.
On Mon, May 14, 2012 at 09:36:55AM +0200, Hans Schillstrom wrote:
> On Saturday 12 May 2012 13:00:03 Dan Carpenter wrote:
> > There is a typo in the error checking and "&&" was used instead of "||".
> > If skb_header_pointer() returns NULL then it leads to a NULL
> > dereference.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> Ack-by: Hans Schillstrom <hans.schillstrom@ericsson.com>
>  
> > ---
> > Btw, this is new code and Sparse complains about endian bugs.
> 
> Can you give me some hints here, arch , compiler version etc.
> I guess it was input to hmark_addr_mask() that complains ?
> 

Yes.  That was one of the warnings.

http://lwn.net/Articles/205624/

net/netfilter/xt_HMARK.c:87:35: warning: incorrect type in assignment (different base types)
net/netfilter/xt_HMARK.c:87:35:    expected unsigned short [unsigned] [usertype] src
net/netfilter/xt_HMARK.c:87:35:    got restricted __be16 [usertype] all
net/netfilter/xt_HMARK.c:88:35: warning: incorrect type in assignment (different base types)
net/netfilter/xt_HMARK.c:88:35:    expected unsigned short [unsigned] [usertype] dst
net/netfilter/xt_HMARK.c:88:35:    got restricted __be16 [usertype] all
net/netfilter/xt_HMARK.c:181:35: warning: incorrect type in argument 1 (different base types)
net/netfilter/xt_HMARK.c:181:35:    expected unsigned int const [usertype] *addr32
net/netfilter/xt_HMARK.c:181:35:    got restricted __be32 *<noident>
net/netfilter/xt_HMARK.c:182:35: warning: incorrect type in argument 1 (different base types)
net/netfilter/xt_HMARK.c:182:35:    expected unsigned int const [usertype] *addr32
net/netfilter/xt_HMARK.c:182:35:    got restricted __be32 *<noident>
net/netfilter/xt_HMARK.c:261:16: warning: invalid assignment: &=
net/netfilter/xt_HMARK.c:261:16:    left side has type unsigned int
net/netfilter/xt_HMARK.c:261:16:    right side has type restricted __be32
net/netfilter/xt_HMARK.c:262:16: warning: invalid assignment: &=
net/netfilter/xt_HMARK.c:262:16:    left side has type unsigned int
net/netfilter/xt_HMARK.c:262:16:    right side has type restricted __be32

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/netfilter/xt_HMARK.c b/net/netfilter/xt_HMARK.c
index 32fbd73..5817d03 100644
--- a/net/netfilter/xt_HMARK.c
+++ b/net/netfilter/xt_HMARK.c
@@ -223,7 +223,7 @@  static int get_inner_hdr(const struct sk_buff *skb, int iphsz, int *nhoff)
 
 	/* Not enough header? */
 	icmph = skb_header_pointer(skb, *nhoff + iphsz, sizeof(_ih), &_ih);
-	if (icmph == NULL && icmph->type > NR_ICMP_TYPES)
+	if (icmph == NULL || icmph->type > NR_ICMP_TYPES)
 		return 0;
 
 	/* Error message? */