Skip to contentSkip to conversation

net/llc/llc_conn.c: fix possible NULL dereference

PATCH In review Eugene Teo 4 participants 8 messages May 19, 2007

Subsystems (3)
Networking Network drivers LLC (802.2)
lore
Share & tools
lore

Eugene Teo

author
#0 lore raw

PATCH 1/1 net/llc/llc_conn.c: fix possible NULL dereference

skb_peek() might return an empty list. skb should be checked before calling llc_pdu_sn_hdr() with it.

Spotted by the Coverity checker.

Signed-off-by Eugene Teo <[email protected]>
net/llc/llc_conn.c +0 -0
index 3b8cfbe..28a3994 100644
@@ -323,7 +323,8 @@ int llc_conn_remove_acked_pdus(struct sock *sk, u8 nr, u16

*how_many_unacked)

        if (!q_len)
                goto out;
-       skb = skb_peek(&llc->pdu_unack_q);
+       if (! (skb = skb_peek(&llc->pdu_unack_q)))
+               goto out;
        pdu = llc_pdu_sn_hdr(skb);

        /* finding position of last acked pdu in queue */
#1 lore raw
Quoted 6 lines · On Sat, 19 May 2007 13:13:07 +0800 Eugene Teo wrote:

skb_peek() might return an empty list. skb should be checked before calling llc_pdu_sn_hdr() with it.

Spotted by the Coverity checker.

Signed-off-by Eugene Teo <[email protected]>

Hi Eugene,

Networking patches need to be sent to the [email protected] mailing list (and lkml can be omitted IMHO).

But... instead of doing the assignment and test in one swoop, we prefer:

Review

Quoted 6 lines
        if (!q_len)
                goto out;
        skb = skb_peek(&llc->pdu_unack_q);
(quoted diff) +2 -0
+       if (!skb)
+               goto out;
        pdu = llc_pdu_sn_hdr(skb);

Oh, and your patch has spaces instead of tabs. It's a hassle to get thunderbird to send a patch that preserves tabs. See if this:
http://mbligh.org/linuxdocs/Email/Clients/Thunderbird
helps you any.

Quoted 16 lines
net/llc/llc_conn.c +2 -1
index 3b8cfbe..28a3994 100644
@@ -323,7 +323,8 @@ int llc_conn_remove_acked_pdus(struct sock *sk, u8 nr, u16
 *how_many_unacked)
 
        if (!q_len)
                goto out;
-       skb = skb_peek(&llc->pdu_unack_q);
+       if (! (skb = skb_peek(&llc->pdu_unack_q)))
+               goto out;
        pdu = llc_pdu_sn_hdr(skb);
 

        /* finding position of last acked pdu in queue */
Quoted 1 line

-

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

author
#2 lore raw

Hi Randy,

Quoted 8 lines · Randy Dunlap
Quoted 6 lines · On Sat, 19 May 2007 13:13:07 +0800 Eugene Teo wrote:

skb_peek() might return an empty list. skb should be checked before calling llc_pdu_sn_hdr() with it.

Spotted by the Coverity checker.

Signed-off-by Eugene Teo <[email protected]>

[...]

Quoted 6 lines

Networking patches need to be sent to the [email protected] mailing list (and lkml can be omitted IMHO).

But... instead of doing the assignment and test in one swoop, we prefer:

Right, thanks for the reminder!

Eugene

author
#3 lore raw
In reply to Randy Dunlap
Quoted 8 lines · Randy Dunlap
Quoted 6 lines · On Sat, 19 May 2007 13:13:07 +0800 Eugene Teo wrote:

skb_peek() might return an empty list. skb should be checked before calling llc_pdu_sn_hdr() with it.

Spotted by the Coverity checker.

Signed-off-by Eugene Teo <[email protected]>

[...]

Quoted 5 lines

Oh, and your patch has spaces instead of tabs. It's a hassle to get thunderbird to send a patch that preserves tabs. See if this:
http://mbligh.org/linuxdocs/Email/Clients/Thunderbird
helps you any.

Here's a resend:

skb_peek() might return an empty list. skb should be checked before calling llc_pdu_sn_hdr() with it.

Spotted by the Coverity checker.

Signed-off-by Eugene Teo <[email protected]>
net/llc/llc_conn.c +2 -0
index 3b8cfbe..6d3a07e 100644
@@ -324,6 +324,8 @@ int llc_conn_remove_acked_pdus(struct sock *sk, u8 nr, u16 *how_many_unacked)
 	if (!q_len)
 		goto out;
 	skb = skb_peek(&llc->pdu_unack_q);
+	if (!skb)
+		goto out;
 	pdu = llc_pdu_sn_hdr(skb);
 
 	/* finding position of last acked pdu in queue */
maintainer
#4 lore raw

From: Eugene Teo <[email protected]>
Date: Sat, 19 May 2007 13:49:11 +0800

Quoted 1 line

Spotted by the Coverity checker.

Why am I not surprised :-(

There is no bug here, if Coverity warns every single time skb_peek() is used and not tested against NULL, that's a very serious shortcoming of Coverity or what it has been taught about SKB queues.

It needs to learn that skb_queue_len() != 0 implies skb_peek() will not return NULL sans locking errors.

maintainer
#5 lore raw
In reply to Randy Dunlap

From: Randy Dunlap <[email protected]>
Date: Fri, 18 May 2007 22:30:05 -0700

Quoted 5 lines

Networking patches need to be sent to the [email protected] mailing list (and lkml can be omitted IMHO).

But... instead of doing the assignment and test in one swoop, we prefer:

In any event the patch is totally bogus because the code checks to make sure skb_queue_len() != 0 first so NULL cannot occur.

#6 lore raw
In reply to Eugene Teo

net/llc/llc_conn.c int llc_conn_remove_acked_pdus(struct sock *sk, u8 nr, u16 · line 328

Quoted 13 lines · Eugene Teo
net/llc/llc_conn.c +2 -1
index 3b8cfbe..28a3994 100644
@@ -323,7 +323,8 @@ int llc_conn_remove_acked_pdus(struct sock *sk, u8 nr, u16
 *how_many_unacked)
 
       if (!q_len)
               goto out;
-       skb = skb_peek(&llc->pdu_unack_q);
+       if (! (skb = skb_peek(&llc->pdu_unack_q)))
+               goto out;

Actually we just checked that the queue length is non-zero so there must be a packet there unless someone's just removed it. If it were possible for someone else to remove it in parallel, then we've got bigger problems to worry about :)

Cheers,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
maintainer
#7 lore raw
In reply to Eugene Teo

From: Eugene Teo <[email protected]>
Date: Sat, 19 May 2007 13:13:07 +0800

Quoted 6 lines

skb_peek() might return an empty list. skb should be checked before calling llc_pdu_sn_hdr() with it.

Spotted by the Coverity checker.

Signed-off-by Eugene Teo <[email protected]>

The code checks skb_queue_len() for zero first, therefore NULL is impossible.

Can you check this kind of stuff before submitting patches like this?

Thank you.