Message ID | 1300988662-13204-1-git-send-email-jluebbe@debian.org |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
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
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; }
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(-)