[B.A.T.M.A.N.] [PATCH next 1/2] batman-adv: check return type of unicast packet preparations
batadv_send_skb_prepare_unicast(_4addr) might reallocate the skb's data. And if it tries to do so then this can potentially fail.
We shouldn't continue working on this skb in such a case.
Signed-off-by: Linus Lüssing linus.luessing@web.de --- unicast.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/unicast.c b/unicast.c index dc8b5d4..4c5a1aa 100644 --- a/unicast.c +++ b/unicast.c @@ -428,11 +428,13 @@ find_router:
switch (packet_type) { case BATADV_UNICAST: - batadv_unicast_prepare_skb(skb, orig_node); + if (!batadv_unicast_prepare_skb(skb, orig_node)) + goto out; break; case BATADV_UNICAST_4ADDR: - batadv_unicast_4addr_prepare_skb(bat_priv, skb, orig_node, - packet_subtype); + if (!batadv_unicast_4addr_prepare_skb(bat_priv, skb, orig_node, + packet_subtype)) + goto out; break; default: /* this function supports UNICAST and UNICAST_4ADDR only. It
batadv_send_skb_prepare_unicast(_4addr) might reallocate the skb's data. If it does then our ethhdr pointer is not valid anymore in batadv_send_skb_unicast(), resulting in a kernel paging error.
This patch fixes this issue by storing the few bytes we are interested in on the stack before modifying the skb.
Signed-off-by: Linus Lüssing linus.luessing@web.de --- unicast.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/unicast.c b/unicast.c index 4c5a1aa..c636fd5 100644 --- a/unicast.c +++ b/unicast.c @@ -395,6 +395,7 @@ int batadv_unicast_generic_send_skb(struct batadv_priv *bat_priv, struct sk_buff *skb, int packet_type, int packet_subtype) { + uint8_t dest[ETH_ALEN]; struct ethhdr *ethhdr = (struct ethhdr *)skb->data; struct batadv_unicast_packet *unicast_packet; struct batadv_orig_node *orig_node; @@ -426,6 +427,8 @@ find_router: if (!neigh_node) goto out;
+ memcpy(dest, ethhdr->h_dest, sizeof(dest)); + switch (packet_type) { case BATADV_UNICAST: if (!batadv_unicast_prepare_skb(skb, orig_node)) @@ -450,7 +453,7 @@ find_router: * try to reroute it because the ttvn contained in the header is less * than the current one */ - if (batadv_tt_global_client_is_roaming(bat_priv, ethhdr->h_dest)) + if (batadv_tt_global_client_is_roaming(bat_priv, dest)) unicast_packet->ttvn = unicast_packet->ttvn - 1;
dev_mtu = neigh_node->if_incoming->net_dev->mtu;
Il 27.07.2013 03:24 Linus Lüssing ha scritto:
batadv_send_skb_prepare_unicast(_4addr) might reallocate the skb's data. If it does then our ethhdr pointer is not valid anymore in batadv_send_skb_unicast(), resulting in a kernel paging error.
This patch fixes this issue by storing the few bytes we are interested in on the stack before modifying the skb.
Good fix! thanks!
However, I think it would be nice to send another patch aiming master which could polish this situation a bit better: e.g. by calling skb_reset_mac_header() in the batadv_send_skb_prepare_unicast_* functions and then get the Ethernet header with eth_hdr() right after having changed the skb.
Signed-off-by: Linus Lüssing linus.luessing@web.de
Acked-by: Antonio Quartulli ordex@autistici.org
On Monday, July 29, 2013 04:17:57 Antonio Quartulli wrote:
Il 27.07.2013 03:24 Linus Lüssing ha scritto:
batadv_send_skb_prepare_unicast(_4addr) might reallocate the skb's data. If it does then our ethhdr pointer is not valid anymore in batadv_send_skb_unicast(), resulting in a kernel paging error.
This patch fixes this issue by storing the few bytes we are interested in on the stack before modifying the skb.
Good fix! thanks!
However, I think it would be nice to send another patch aiming master which could polish this situation a bit better: e.g. by calling skb_reset_mac_header() in the batadv_send_skb_prepare_unicast_* functions and then get the Ethernet header with eth_hdr() right after having changed the skb.
I like that approach because it seems cleaner that way. Is there a reason not do it right away ?
Cheers, Marek
On Fri, Aug 02, 2013 at 12:05:28PM +0800, Marek Lindner wrote:
On Monday, July 29, 2013 04:17:57 Antonio Quartulli wrote:
Il 27.07.2013 03:24 Linus Lüssing ha scritto:
batadv_send_skb_prepare_unicast(_4addr) might reallocate the skb's data. If it does then our ethhdr pointer is not valid anymore in batadv_send_skb_unicast(), resulting in a kernel paging error.
This patch fixes this issue by storing the few bytes we are interested in on the stack before modifying the skb.
Good fix! thanks!
However, I think it would be nice to send another patch aiming master which could polish this situation a bit better: e.g. by calling skb_reset_mac_header() in the batadv_send_skb_prepare_unicast_* functions and then get the Ethernet header with eth_hdr() right after having changed the skb.
I like that approach because it seems cleaner that way. Is there a reason not do it right away ?
I thought the second approach would consist in a bigger patch and so I preferred to send this to net and the bigger patch to net-next later.
But you may be right. The change I suggested is not really big. Linus, would you like to provide "the next" patch so that we can clearly understand if it is worth sending to net or not?
Cheers,
On Fri, Aug 02, 2013 at 09:18:06AM +0200, Antonio Quartulli wrote:
On Fri, Aug 02, 2013 at 12:05:28PM +0800, Marek Lindner wrote:
On Monday, July 29, 2013 04:17:57 Antonio Quartulli wrote:
Il 27.07.2013 03:24 Linus Lüssing ha scritto:
batadv_send_skb_prepare_unicast(_4addr) might reallocate the skb's data. If it does then our ethhdr pointer is not valid anymore in batadv_send_skb_unicast(), resulting in a kernel paging error.
This patch fixes this issue by storing the few bytes we are interested in on the stack before modifying the skb.
Good fix! thanks!
However, I think it would be nice to send another patch aiming master which could polish this situation a bit better: e.g. by calling skb_reset_mac_header() in the batadv_send_skb_prepare_unicast_* functions and then get the Ethernet header with eth_hdr() right after having changed the skb.
I like that approach because it seems cleaner that way. Is there a reason not do it right away ?
I thought the second approach would consist in a bigger patch and so I preferred to send this to net and the bigger patch to net-next later.
But you may be right. The change I suggested is not really big. Linus, would you like to provide "the next" patch so that we can clearly understand if it is worth sending to net or not?
Sure! I think it could actually be a lot easier than I thought (if I didn't misread or miss something in the skb code).
Cheers,
-- Antonio Quartulli
..each of us alone is worth nothing.. Ernesto "Che" Guevara
Il 27.07.2013 03:24 Linus Lüssing ha scritto:
batadv_send_skb_prepare_unicast(_4addr) might reallocate the skb's data.
In next these functions are named a bit differently. I think this can be fixed while merging the patch.
And if it tries to do so then this can potentially fail.
We shouldn't continue working on this skb in such a case.
Signed-off-by: Linus Lüssing linus.luessing@web.de
Acked-by: Antonio Quartulli ordex@autistici.org
On Monday, July 29, 2013 04:14:23 Antonio Quartulli wrote:
Il 27.07.2013 03:24 Linus Lüssing ha scritto:
batadv_send_skb_prepare_unicast(_4addr) might reallocate the skb's data.
In next these functions are named a bit differently. I think this can be fixed while merging the patch.
And if it tries to do so then this can potentially fail.
We shouldn't continue working on this skb in such a case.
Signed-off-by: Linus Lüssing linus.luessing@web.de
Acked-by: Antonio Quartulli ordex@autistici.org
Applied in revision d781a70.
Thanks, Marek
b.a.t.m.a.n@lists.open-mesh.org
-
Antonio Quartulli -
Linus Lüssing -
Marek Lindner