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

login
register
mail settings
Submitter Jan Luebbe
Date March 24, 2011, 5:44 p.m.
Message ID <1300988662-13204-1-git-send-email-jluebbe@debian.org>
Download mbox | patch
Permalink /patch/88260/
State Accepted
Delegated to: David Miller
Headers show

Comments

Jan Luebbe - March 24, 2011, 5:44 p.m.
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(-)
David Miller - March 24, 2011, 11:20 p.m.
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
Jan Luebbe - March 25, 2011, 6:11 a.m.
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
David Miller - March 28, 2011, 1:32 a.m.
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

Patch

diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c
index 1906fa3..28a736f 100644
--- a/net/ipv4/ip_options.c
+++ b/net/ipv4/ip_options.c
@@ -140,11 +140,11 @@  int ip_options_echo(struct ip_options * dopt, struct sk_buff * skb)
 				} else {
 					dopt->ts_needtime = 0;
 
-					if (soffset + 8 <= optlen) {
+					if (soffset + 7 <= optlen) {
 						__be32 addr;
 
-						memcpy(&addr, sptr+soffset-1, 4);
-						if (inet_addr_type(dev_net(skb_dst(skb)->dev), addr) != RTN_LOCAL) {
+						memcpy(&addr, dptr+soffset-1, 4);
+						if (inet_addr_type(dev_net(skb_dst(skb)->dev), addr) != RTN_UNICAST) {
 							dopt->ts_needtime = 1;
 							soffset += 8;
 						}