IPv4: ip_options_compile() how can we avoid blowing up on a NULL skb???

mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed

* IPv4: ip_options_compile() how can we avoid blowing up on a NULL skb???
@ 2006-11-16 22:34 Jesper Juhl
  2006-11-16 23:47 ` David Miller
  2006-11-17  0:22 ` Herbert Xu
  0 siblings, 2 replies; 6+ messages in thread
From: Jesper Juhl @ 2006-11-16 22:34 UTC (permalink / raw)
  To: netdev; +Cc: David Miller, Linux Kernel Mailing List

Hi,

In net/ipv4/ip_options.c::ip_options_compile() we have the following
code at the start of the function :


int ip_options_compile(struct ip_options * opt, struct sk_buff * skb)
{
        int l;
        unsigned char * iph;
        unsigned char * optptr;
        int optlen;
        unsigned char * pp_ptr = NULL;
        struct rtable *rt = skb ? (struct rtable*)skb->dst : NULL;

        if (!opt) {
                opt = &(IPCB(skb)->opt);
                iph = skb->nh.raw;
                opt->optlen = ((struct iphdr *)iph)->ihl*4 -
sizeof(struct iphdr);
                optptr = iph + sizeof(struct iphdr);
                opt->is_data = 0;
        } else {
                optptr = opt->is_data ? opt->__data : (unsigned
char*)&(skb->nh.iph[1]);
                iph = optptr - sizeof(struct iphdr);
        }

...

I don't see how that avoids blowing up if we get passed a NULL skb.

>From the line
    struct rtable *rt = skb ? (struct rtable*)skb->dst : NULL;
it is clear that we /may/ get passed a null skb.

Then a bit further down in the  if (!opt)  bit we dereference 'skb' :
    opt = &(IPCB(skb)->opt);
and we also may dereference it in the  else  part :
    optptr = opt->is_data ? opt->__data : (unsigned char*)&(skb->nh.iph[1]);

So if 'skb' is NULL, the only route I see that doesn't cause a NULL
pointer deref is if  (opt != NULL)  and at the same time
(opt->is_data != NULL)  .   Is that guaranteed in any way?  If now,
how come we don't blow up regularly?


-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: IPv4: ip_options_compile() how can we avoid blowing up on a NULL skb???
  2006-11-16 22:34 IPv4: ip_options_compile() how can we avoid blowing up on a NULL skb??? Jesper Juhl
@ 2006-11-16 23:47 ` David Miller
  2006-11-17  0:37   ` Herbert Xu
  2006-11-17  0:22 ` Herbert Xu
  1 sibling, 1 reply; 6+ messages in thread
From: David Miller @ 2006-11-16 23:47 UTC (permalink / raw)
  To: jesper.juhl; +Cc: netdev, linux-kernel

From: "Jesper Juhl" <jesper.juhl@gmail.com>
Date: Thu, 16 Nov 2006 23:34:14 +0100

> So if 'skb' is NULL, the only route I see that doesn't cause a NULL
> pointer deref is if  (opt != NULL)  and at the same time
> (opt->is_data != NULL)  .   Is that guaranteed in any way?

Yes, it is guarenteed, all callers  make sure these invariants
hold true.

I'm very happy to accept a patch which assert's this using BUG()
checks :-)

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: IPv4: ip_options_compile() how can we avoid blowing up on a NULL skb???
  2006-11-16 23:47 ` David Miller
@ 2006-11-17  0:37   ` Herbert Xu
  2006-11-17  3:46     ` Dmitry Torokhov
  0 siblings, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2006-11-17  0:37 UTC (permalink / raw)
  To: David Miller; +Cc: jesper.juhl, netdev, linux-kernel

David Miller <davem@davemloft.net> wrote:
>
> I'm very happy to accept a patch which assert's this using BUG()
> checks :-)

A BUG() won't be necessary because the NULL pointer dereferences will
OOPS anyway.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: IPv4: ip_options_compile() how can we avoid blowing up on a NULL skb???
  2006-11-17  0:37   ` Herbert Xu
@ 2006-11-17  3:46     ` Dmitry Torokhov
  2006-11-17  5:28       ` Herbert Xu
  0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2006-11-17  3:46 UTC (permalink / raw)
  To: Herbert Xu; +Cc: David Miller, jesper.juhl, netdev, linux-kernel

On Thursday 16 November 2006 19:37, Herbert Xu wrote:
> David Miller <davem@davemloft.net> wrote:
> >
> > I'm very happy to accept a patch which assert's this using BUG()
> > checks :-)
> 
> A BUG() won't be necessary because the NULL pointer dereferences will
> OOPS anyway.
>

BUG()s there would be a mechanism to document invariants so next time
someone is looking at the code there are no questions.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: IPv4: ip_options_compile() how can we avoid blowing up on a NULL skb???
  2006-11-17  3:46     ` Dmitry Torokhov
@ 2006-11-17  5:28       ` Herbert Xu
  0 siblings, 0 replies; 6+ messages in thread
From: Herbert Xu @ 2006-11-17  5:28 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: herbert, davem, jesper.juhl, netdev, linux-kernel

Dmitry Torokhov <dtor@insightbb.com> wrote:
> 
> BUG()s there would be a mechanism to document invariants so next time
> someone is looking at the code there are no questions.

Well if someone is documenting this then wouldn't a comment (or even a
kerneldoc style block) be better?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: IPv4: ip_options_compile() how can we avoid blowing up on a NULL skb???
  2006-11-16 22:34 IPv4: ip_options_compile() how can we avoid blowing up on a NULL skb??? Jesper Juhl
  2006-11-16 23:47 ` David Miller
@ 2006-11-17  0:22 ` Herbert Xu
  1 sibling, 0 replies; 6+ messages in thread
From: Herbert Xu @ 2006-11-17  0:22 UTC (permalink / raw)
  To: Jesper Juhl; +Cc: netdev, davem, linux-kernel

Jesper Juhl <jesper.juhl@gmail.com> wrote:
> 
> So if 'skb' is NULL, the only route I see that doesn't cause a NULL
> pointer deref is if  (opt != NULL)  and at the same time
> (opt->is_data != NULL)  .   Is that guaranteed in any way?  If now,
> how come we don't blow up regularly?

Yes that's how it's supposed to be used.  This function either constructs
an opts structure from a packet, or it verifies the validity of a suspect
opts structure (without a packet).  In the latter case, both opt and
opt->is_data should be non-zero.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2006-11-17  5:28 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-11-16 22:34 IPv4: ip_options_compile() how can we avoid blowing up on a NULL skb??? Jesper Juhl
2006-11-16 23:47 ` David Miller
2006-11-17  0:37   ` Herbert Xu
2006-11-17  3:46     ` Dmitry Torokhov
2006-11-17  5:28       ` Herbert Xu
2006-11-17  0:22 ` Herbert Xu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®