Fix IP timestamp option (IPOPT_TS_PRESPEC) handling in ip_options_echo()

The current handling of echoed IP timestamp options with prespecified
addresses is rather broken since the 2.2.x kernels. As far as i understand
it, it should behave like when originating packets.

Currently it will only timestamp the next free slot if:
 - there is space for *two* timestamps
 - some random data from the echoed packet taken as an IP is *not* a local IP

This first is caused by an off-by-one error. 'soffset' points to the next
free slot and so we only need to have 'soffset + 7 <= optlen'.

The second bug is using sptr as the start of the option, when it really is
set to 'skb_network_header(skb)'. I just use dptr instead which points to
the timestamp option.

Finally it would only timestamp for non-local IPs, which we shouldn't do.
So instead we exclude all unicast destinations, similar to what we do in
ip_options_compile().

Signed-off-by: Jan Luebbe <jluebbe@debian.org>
---
 net/ipv4/ip_options.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

From: Jan Luebbe <jluebbe@debian.org>
Date: Thu, 24 Mar 2011 18:44:22 +0100

> -					if (soffset + 8 <= optlen) {
> +					if (soffset + 7 <= optlen) {

I don't see how you can legally reduce this check from 8 to 7 bytes.

> +						if (inet_addr_type(dev_net(skb_dst(skb)->dev), addr) != RTN_UNICAST) {
>  							dopt->ts_needtime = 1;
>  							soffset += 8;
>  						}

Yet keep this code which advances soffset by 8.
--
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

Hi!

On Thu, 2011-03-24 at 16:20 -0700, David Miller wrote: 
> From: Jan Luebbe <jluebbe@debian.org>
> Date: Thu, 24 Mar 2011 18:44:22 +0100
> 
> > -					if (soffset + 8 <= optlen) {
> > +					if (soffset + 7 <= optlen) {
> 
> I don't see how you can legally reduce this check from 8 to 7 bytes.
> 
> > +						if (inet_addr_type(dev_net(skb_dst(skb)->dev), addr) != RTN_UNICAST) {
> >  							dopt->ts_needtime = 1;
> >  							soffset += 8;
> >  						}
> 
> Yet keep this code which advances soffset by 8.

The encoding of soffset is a bit unusual, in that the option is 'full'
when soffset = optlen + 1. We need to keep advancing soffset by 8
because that's really the amount of data per entry.

In ip_options_compile, near 'case IPOPT_TS_PRESPEC:' (line 382 in
2.6.38), we have: 
      case IPOPT_TS_PRESPEC:
        if (optptr[2]+7 > optptr[1]) {  
                pp_ptr = optptr + 2;            
                goto error;
        }
        opt->ts = optptr - iph;
        {
                __be32 addr;
                memcpy(&addr, &optptr[optptr[2]-1], 4);
                if (inet_addr_type(net, addr) == RTN_UNICAST)
                        break;
                if (skb)
                        timeptr = (__be32*)&optptr[optptr[2]+3];
        }
        opt->ts_needtime = 1;
        optptr[2] += 8;
        break;

Here optptr[1] matches optlen from _echo and optptr[2] is soffset.

When we use soffset to index into the packet we substract 1. See the
memcpy in my patch which reads from the packet and also the memcpys in
_compile around line 390.

If we checked soffset + 8 <= optlen, we abort updating the timestamp
when exactly space for one entry remains.

Also, I don't think writing to unallocated memory is possible when the
IP header in the echoed packet are doesn't have enough space for the
length indicated by optlen, as we clear dopt with memset(dopt, 0,
sizeof(struct ip_options)) so it dopt must have the right size already.

Best regards,
Jan

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

From: Jan Lübbe <jluebbe@debian.org>
Date: Fri, 25 Mar 2011 07:11:12 +0100

> The encoding of soffset is a bit unusual, in that the option is 'full'
> when soffset = optlen + 1. We need to keep advancing soffset by 8
> because that's really the amount of data per entry.

Ok, this code is officially terrible :-)

Thanks for explaining things, I've applied your patch, thanks.
--
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