* [PATCH] ipv6: check if dereference of ipv6 header is safe
@ 2013-01-17 3:56 Hannes Frederic Sowa
2013-01-18 2:06 ` Hannes Frederic Sowa
0 siblings, 1 reply; 6+ messages in thread
From: Hannes Frederic Sowa @ 2013-01-17 3:56 UTC (permalink / raw)
To: netdev
When ipip6_rcv gets called we are sure that we have a full blown
ipv4 packet header in the linear skb buffer (this is checked by
xfrm4_mode_tunnel_input). Because we dereference fields of the inner
ipv6 header we should actually check for the length of the sum of the
ipv4 and ipv6 header.
If the skb is too short this packet could very well be destined for
another tunnel. So we should notify the caller accordingly (albeit
currently xfrm4_mode_tunnel_input does not care; this could need another
patch).
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
net/ipv6/sit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index 2b4c15a..389d6e3 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -612,8 +612,8 @@ static int ipip6_rcv(struct sk_buff *skb)
struct ip_tunnel *tunnel;
int err;
- if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
- goto out;
+ if (!pskb_may_pull(skb, sizeof(struct iphdr) + sizeof(struct ipv6hdr)))
+ return 1;
iph = ip_hdr(skb);
--
1.7.11.7
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ipv6: check if dereference of ipv6 header is safe
2013-01-17 3:56 [PATCH] ipv6: check if dereference of ipv6 header is safe Hannes Frederic Sowa
@ 2013-01-18 2:06 ` Hannes Frederic Sowa
2013-01-18 2:08 ` David Miller
2013-01-18 2:21 ` Eric Dumazet
0 siblings, 2 replies; 6+ messages in thread
From: Hannes Frederic Sowa @ 2013-01-18 2:06 UTC (permalink / raw)
To: netdev
On Thu, Jan 17, 2013 at 04:56:52AM +0100, Hannes Frederic Sowa wrote:
> When ipip6_rcv gets called we are sure that we have a full blown
> ipv4 packet header in the linear skb buffer (this is checked by
> xfrm4_mode_tunnel_input). Because we dereference fields of the inner
> ipv6 header we should actually check for the length of the sum of the
> ipv4 and ipv6 header.
>
> If the skb is too short this packet could very well be destined for
> another tunnel. So we should notify the caller accordingly (albeit
> currently xfrm4_mode_tunnel_input does not care; this could need another
> patch).
While grepping for xfrm_tunnel and handler I studied the wrong
call stack. ipip6_rcv is not called by xfrm_mode_tunnel_input but by
tunnel64_rcv. This function already ensures that we can safely dereference
the ipv6 header. I got confused by xfrm_mode_tunnel only checking for
the size of an ipv4 header and assumed that the data section of the
skb had not been rolled forward. This patch brings ipip6_rcv in line
with ipip_rcv.
This patch superseds the old one:
[PATCH] ipv6: remove unneeded check to pskb_may_pull
This is already checked by the caller (tunnel64_rcv) and brings ipip6_rcv
in line with ipip_rcv.
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
net/ipv6/sit.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
index cfba99b..98fe536 100644
--- a/net/ipv6/sit.c
+++ b/net/ipv6/sit.c
@@ -592,15 +592,10 @@ out:
static int ipip6_rcv(struct sk_buff *skb)
{
- const struct iphdr *iph;
+ const struct iphdr *iph = ip_hdr(skb);
struct ip_tunnel *tunnel;
int err;
- if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
- goto out;
-
- iph = ip_hdr(skb);
-
tunnel = ipip6_tunnel_lookup(dev_net(skb->dev), skb->dev,
iph->saddr, iph->daddr);
if (tunnel != NULL) {
--
1.7.11.7
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] ipv6: check if dereference of ipv6 header is safe
2013-01-18 2:06 ` Hannes Frederic Sowa
@ 2013-01-18 2:08 ` David Miller
2013-01-18 2:21 ` Eric Dumazet
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2013-01-18 2:08 UTC (permalink / raw)
To: hannes; +Cc: netdev
From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Fri, 18 Jan 2013 03:06:12 +0100
> On Thu, Jan 17, 2013 at 04:56:52AM +0100, Hannes Frederic Sowa wrote:
>> When ipip6_rcv gets called we are sure that we have a full blown
>> ipv4 packet header in the linear skb buffer (this is checked by
>> xfrm4_mode_tunnel_input). Because we dereference fields of the inner
>> ipv6 header we should actually check for the length of the sum of the
>> ipv4 and ipv6 header.
>>
>> If the skb is too short this packet could very well be destined for
>> another tunnel. So we should notify the caller accordingly (albeit
>> currently xfrm4_mode_tunnel_input does not care; this could need another
>> patch).
>
> While grepping for xfrm_tunnel and handler I studied the wrong
> call stack. ipip6_rcv is not called by xfrm_mode_tunnel_input but by
> tunnel64_rcv. This function already ensures that we can safely dereference
> the ipv6 header. I got confused by xfrm_mode_tunnel only checking for
> the size of an ipv4 header and assumed that the data section of the
> skb had not been rolled forward. This patch brings ipip6_rcv in line
> with ipip_rcv.
>
> This patch superseds the old one:
Don't post new patches using replies to old threads. Create entirely
new patch postings.
> [PATCH] ipv6: remove unneeded check to pskb_may_pull
This is an ambiguous commit header line, it doesn't say that you're
modifying the SIT tunnel driver at all. Please fix this.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ipv6: check if dereference of ipv6 header is safe
2013-01-18 2:06 ` Hannes Frederic Sowa
2013-01-18 2:08 ` David Miller
@ 2013-01-18 2:21 ` Eric Dumazet
2013-01-18 3:08 ` Hannes Frederic Sowa
1 sibling, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2013-01-18 2:21 UTC (permalink / raw)
To: Hannes Frederic Sowa; +Cc: netdev
On Fri, 2013-01-18 at 03:06 +0100, Hannes Frederic Sowa wrote:
> [PATCH] ipv6: remove unneeded check to pskb_may_pull
>
> This is already checked by the caller (tunnel64_rcv) and brings ipip6_rcv
> in line with ipip_rcv.
>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> ---
> net/ipv6/sit.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/net/ipv6/sit.c b/net/ipv6/sit.c
> index cfba99b..98fe536 100644
> --- a/net/ipv6/sit.c
> +++ b/net/ipv6/sit.c
> @@ -592,15 +592,10 @@ out:
>
> static int ipip6_rcv(struct sk_buff *skb)
> {
> - const struct iphdr *iph;
> + const struct iphdr *iph = ip_hdr(skb);
> struct ip_tunnel *tunnel;
> int err;
>
> - if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
> - goto out;
> -
> - iph = ip_hdr(skb);
> -
> tunnel = ipip6_tunnel_lookup(dev_net(skb->dev), skb->dev,
> iph->saddr, iph->daddr);
> if (tunnel != NULL) {
But we use a 'struct iphdr' here, not a ipv6hdr
So we basically implicitely rely on sizeof(struct iphdr) <=
sizeof(struct ipv6hdr)
I would leave the pskb_may_pull() call and fix it, even if not really
needed.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ipv6: check if dereference of ipv6 header is safe
2013-01-18 2:21 ` Eric Dumazet
@ 2013-01-18 3:08 ` Hannes Frederic Sowa
2013-01-18 19:12 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Hannes Frederic Sowa @ 2013-01-18 3:08 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
On Thu, Jan 17, 2013 at 06:21:37PM -0800, Eric Dumazet wrote:
> But we use a 'struct iphdr' here, not a ipv6hdr
>
> So we basically implicitely rely on sizeof(struct iphdr) <=
> sizeof(struct ipv6hdr)
>
> I would leave the pskb_may_pull() call and fix it, even if not really
> needed.
Please correct me if I am wrong:
The callstack as captured in ipip6_rcv:
ipip6_rcv+0xcd/0x680 [sit]
tunnel64_rcv+0x4a/0x174 [tunnel4]
ip_local_deliver+0x152/0x470
? ip_local_deliver+0x75/0x470
ip_rcv+0x36d/0x650
ip_rcv does first check if the ipv4 header is complete (inclusive options) and
passes control to ip_local_deliver which calls __skb_pull(skb,
ip_hdrlen(skb)). So ->data is forwarded behind the ipv4 header. The next
pskb_may_pull check would check if the necessary amount of data behind the
ipv4 header is available hence I assume the check in tunnel64_rcv is enough:
109 static int tunnel64_rcv(struct sk_buff *skb)
110 {
111 struct xfrm_tunnel *handler;
112
113 if (!pskb_may_pull(skb, sizeof(struct ipv6hdr)))
114 goto drop;
115
116 for_each_tunnel_rcu(tunnel64_handlers, handler)
117 if (!handler->handler(skb))
118 return 0;
Thanks,
Hannes
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ipv6: check if dereference of ipv6 header is safe
2013-01-18 3:08 ` Hannes Frederic Sowa
@ 2013-01-18 19:12 ` Eric Dumazet
0 siblings, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2013-01-18 19:12 UTC (permalink / raw)
To: Hannes Frederic Sowa; +Cc: netdev
On Fri, 2013-01-18 at 04:08 +0100, Hannes Frederic Sowa wrote:
> Please correct me if I am wrong:
>
> The callstack as captured in ipip6_rcv:
> ipip6_rcv+0xcd/0x680 [sit]
> tunnel64_rcv+0x4a/0x174 [tunnel4]
> ip_local_deliver+0x152/0x470
> ? ip_local_deliver+0x75/0x470
> ip_rcv+0x36d/0x650
>
> ip_rcv does first check if the ipv4 header is complete (inclusive options) and
> passes control to ip_local_deliver which calls __skb_pull(skb,
> ip_hdrlen(skb)). So ->data is forwarded behind the ipv4 header. The next
> pskb_may_pull check would check if the necessary amount of data behind the
> ipv4 header is available hence I assume the check in tunnel64_rcv is enough:
Oh, yes, thats fine.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-01-18 19:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-17 3:56 [PATCH] ipv6: check if dereference of ipv6 header is safe Hannes Frederic Sowa
2013-01-18 2:06 ` Hannes Frederic Sowa
2013-01-18 2:08 ` David Miller
2013-01-18 2:21 ` Eric Dumazet
2013-01-18 3:08 ` Hannes Frederic Sowa
2013-01-18 19:12 ` Eric Dumazet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox