| Message ID | 1382391080-1607-1-git-send-email-antonio@meshcoding.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Antonio Quartulli <antonio@meshcoding.com> Date: Mon, 21 Oct 2013 23:31:20 +0200 > __netpoll_rx() assumes that the data buffer of the received > skb is linear and then passes it to rx_hook(). > However this is not true because the skb has not been > linearized yet. > > This can cause rx_hook() to access non allocated memory > while parsing the received data. > > Fix __netpoll_rx() by explicitly linearising the skb. > > Signed-off-by: Antonio Quartulli <antonio@meshcoding.com> It is rx_hook's obligation to access the SKB properly and not assume that the SKB is linear. It is very expensive to linearize every SKB just for the sake of improperly implemented receive hooks. In particular the rx hooks must make use of interface such as pskb_may_pull(), just like every other protocol does on packet input processing, to make sure the area they want to access is in the linear area. -- 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
On Mon, 2013-10-21 at 23:31 +0200, Antonio Quartulli wrote: > __netpoll_rx() assumes that the data buffer of the received > skb is linear and then passes it to rx_hook(). > However this is not true because the skb has not been > linearized yet. > > This can cause rx_hook() to access non allocated memory > while parsing the received data. > > Fix __netpoll_rx() by explicitly linearising the skb. > > Signed-off-by: Antonio Quartulli <antonio@meshcoding.com> > --- > > I checked linux-3.0 and this bug seems to be already there. Please consider > queueing it for stable. > > > Regards, > > > > net/core/netpoll.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/net/core/netpoll.c b/net/core/netpoll.c > index fc75c9e..97cff18 100644 > --- a/net/core/netpoll.c > +++ b/net/core/netpoll.c > @@ -814,6 +814,9 @@ int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo) > if (pskb_trim_rcsum(skb, len)) > goto out; > > + if (skb_linearize(skb)) > + goto out; > + > iph = (struct iphdr *)skb->data; > if (iph->protocol != IPPROTO_UDP) > goto out; > @@ -855,6 +858,8 @@ int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo) > goto out; > if (pskb_trim_rcsum(skb, len + sizeof(struct ipv6hdr))) > goto out; > + if (skb_linearize(skb)) > + goto out; > ip6h = ipv6_hdr(skb); > if (!pskb_may_pull(skb, sizeof(struct udphdr))) > goto out; Well, if you linearize the skb, no need for pskb_may_pull(), and it would be better to do it once at the beginning... Anyway, how I see nothing sets rx_hook, what am I missing ? # git grep -n rx_hook include/linux/netpoll.h:27: void (*rx_hook)(struct netpoll *, int, char *, int); include/linux/netpoll.h:44: struct list_head rx_np; /* netpolls that registered an rx_hook */ net/core/netpoll.c:639: /* If there are several rx_hooks for the same address, net/core/netpoll.c:722: /* If there are several rx_hooks for the same address, net/core/netpoll.c:837: np->rx_hook(np, ntohs(uh->source), net/core/netpoll.c:875: np->rx_hook(np, ntohs(uh->source), net/core/netpoll.c:1065: if (np->rx_hook) { -- 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: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 21 Oct 2013 15:25:36 -0700 > Anyway, how I see nothing sets rx_hook, what am I missing ? Only out of tree code makes use of this facility, and it's been this way since the facility was introduced. Yes, I'm disappointed and unhappy about this too. -- 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
On Mon, Oct 21, 2013 at 06:23:19PM -0400, David Miller wrote: > From: Antonio Quartulli <antonio@meshcoding.com> > Date: Mon, 21 Oct 2013 23:31:20 +0200 > > > __netpoll_rx() assumes that the data buffer of the received > > skb is linear and then passes it to rx_hook(). > > However this is not true because the skb has not been > > linearized yet. > > > > This can cause rx_hook() to access non allocated memory > > while parsing the received data. > > > > Fix __netpoll_rx() by explicitly linearising the skb. > > > > Signed-off-by: Antonio Quartulli <antonio@meshcoding.com> > > It is rx_hook's obligation to access the SKB properly and not > assume that the SKB is linear. It is very expensive to > linearize every SKB just for the sake of improperly implemented > receive hooks. > > In particular the rx hooks must make use of interface such > as pskb_may_pull(), just like every other protocol does > on packet input processing, to make sure the area they want > to access is in the linear area. > But rx_hook() does not receive any skb: 609 np->rx_hook(np, ntohs(uh->source), 610 (char *)(uh+1), 611 ulen - sizeof(struct udphdr)); it just receives a pointer to the data and can't do anything to make it linear. (uh is a pointer to the udp header). Am I missing something? Regards,
From: Antonio Quartulli <antonio@meshcoding.com> Date: Tue, 22 Oct 2013 08:06:35 +0200 > But rx_hook() does not receive any skb: > > 609 np->rx_hook(np, ntohs(uh->source), > 610 (char *)(uh+1), > 611 ulen - sizeof(struct udphdr)); > > it just receives a pointer to the data and can't do anything to make it linear. > (uh is a pointer to the udp header). Am I missing something? Then this hook's API needs to be fixed, it's completely broken. Make it pass the SKB and the appropriate offset (from skb->data) in bytes. -- 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
On Tue, Oct 22, 2013 at 02:25:25AM -0400, David Miller wrote: > From: Antonio Quartulli <antonio@meshcoding.com> > Date: Tue, 22 Oct 2013 08:06:35 +0200 > > > But rx_hook() does not receive any skb: > > > > 609 np->rx_hook(np, ntohs(uh->source), > > 610 (char *)(uh+1), > > 611 ulen - sizeof(struct udphdr)); > > > > it just receives a pointer to the data and can't do anything to make it linear. > > (uh is a pointer to the udp header). Am I missing something? > > Then this hook's API needs to be fixed, it's completely broken. > > Make it pass the SKB and the appropriate offset (from skb->data) in > bytes. > Ok I will do that. But I think this API change must go into net-next, right? Otherwise we would break every existing user. What about resending this patch to net (after having removed the pskb_may_pull() as pointed out by Eric) and then fixing the API in net-next?
From: Antonio Quartulli <antonio@meshcoding.com> Date: Tue, 22 Oct 2013 08:37:14 +0200 > Ok I will do that. But I think this API change must go into > net-next, right? Otherwise we would break every existing user. Too bad, it's a necessary API change to fix the bug, the users will need to change too. It's not our problem if they are out-of-tree and can't be fixed in-situ. -- 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/core/netpoll.c b/net/core/netpoll.c index fc75c9e..97cff18 100644 --- a/net/core/netpoll.c +++ b/net/core/netpoll.c @@ -814,6 +814,9 @@ int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo) if (pskb_trim_rcsum(skb, len)) goto out; + if (skb_linearize(skb)) + goto out; + iph = (struct iphdr *)skb->data; if (iph->protocol != IPPROTO_UDP) goto out; @@ -855,6 +858,8 @@ int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo) goto out; if (pskb_trim_rcsum(skb, len + sizeof(struct ipv6hdr))) goto out; + if (skb_linearize(skb)) + goto out; ip6h = ipv6_hdr(skb); if (!pskb_may_pull(skb, sizeof(struct udphdr))) goto out;
__netpoll_rx() assumes that the data buffer of the received skb is linear and then passes it to rx_hook(). However this is not true because the skb has not been linearized yet. This can cause rx_hook() to access non allocated memory while parsing the received data. Fix __netpoll_rx() by explicitly linearising the skb. Signed-off-by: Antonio Quartulli <antonio@meshcoding.com> --- I checked linux-3.0 and this bug seems to be already there. Please consider queueing it for stable. Regards, net/core/netpoll.c | 5 +++++ 1 file changed, 5 insertions(+)