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 <[email protected]>
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
From: "Jesper Juhl" <[email protected]>
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 :-)
Jesper Juhl <[email protected]> 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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
David Miller <[email protected]> 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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Thursday 16 November 2006 19:37, Herbert Xu wrote:
> David Miller <[email protected]> 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
Dmitry Torokhov <[email protected]> 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~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt