2007-05-19 05:15:21

by Eugene Teo

Subject: [2.6 patch] 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]>

diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
index 3b8cfbe..28a3994 100644
--- a/net/llc/llc_conn.c
+++ b/net/llc/llc_conn.c
@@ -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 */

2007-05-19 05:24:57

by Randy Dunlap

Subject: Re: [2.6 patch] net/llc/llc_conn.c: fix possible NULL dereference

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:

> if (!q_len)
> goto out;
> skb = skb_peek(&llc->pdu_unack_q);
> + 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.


> diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
> index 3b8cfbe..28a3994 100644
> --- a/net/llc/llc_conn.c
> +++ b/net/llc/llc_conn.c
> @@ -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 */
>
> -

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

2007-05-19 05:43:49

by Herbert Xu

Subject: Re: [2.6 patch] net/llc/llc_conn.c: fix possible NULL dereference

Eugene Teo <[email protected]> wrote:
>
> diff --git a/net/llc/llc_conn.c b/net/llc/llc_conn.c
> index 3b8cfbe..28a3994 100644
> --- a/net/llc/llc_conn.c
> +++ b/net/llc/llc_conn.c
> @@ -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,
--
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

2007-05-19 05:47:17

by Eugene Teo

Subject: Re: [2.6 patch] net/llc/llc_conn.c: fix possible NULL dereference

Hi Randy,

Randy Dunlap wrote:
> 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]>
[...]
>
> 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

2007-05-19 05:59:34

by David Miller

Subject: Re: [2.6 patch] net/llc/llc_conn.c: fix possible NULL dereference

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

> 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.

2007-05-19 06:01:32

by David Miller

Subject: Re: [2.6 patch] net/llc/llc_conn.c: fix possible NULL dereference

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

> 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.