batman-adv: check return type of unicast packet preparations

PATCH Applied Linus Lüssing 3 participants 8 messages Jul 27, 2013 – Aug 2, 2013

Share & tools

Linus Lüssing

author

2013-07-27 01:24 UTC #0 lore raw

PATCH 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 <[email protected]>

---
 unicast.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

unicast.c +5 -3

index dc8b5d4..4c5a1aa 100644
@@ -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

Signature · 1 line

1.7.10.4

Linus Lüssing

author

2013-07-27 01:24 UTC #1 lore raw

PATCH 2/2 batman-adv: fix potential kernel paging error for unicast transmissions

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 <[email protected]>

---
 unicast.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

unicast.c +4 -1

index 4c5a1aa..c636fd5 100644
@@ -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;

Signature · 1 line

1.7.10.4

Antonio Quartulli

Acked-by on v1 2/2

2013-07-28 20:17 UTC #2 lore raw

Re: [B.A.T.M.A.N.] [PATCH next 2/2] batman-adv: fix potential kernel paging error for unicast transmissions

Il 27.07.2013 03:24 Linus Lüssing ha scritto:

Quoted 6 lines

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.

Quoted 2 lines

Signed-off-by Linus Lüssing <[email protected]>

Acked-by Antonio Quartulli <[email protected]>

Signature · 1 line

Antonio Quartulli

Marek Lindner

2013-08-02 04:05 UTC #3 lore raw

Re: [B.A.T.M.A.N.] [PATCH next 2/2] batman-adv: fix potential kernel paging error for unicast transmissions

Quoted 8 lines · On Monday, July 29, 2013 04:17:57 Antonio Quartulli wrote:

Il 27.07.2013 03:24 Linus Lüssing ha scritto:
Quoted 6 lines
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.

Quoted 8 lines

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

Antonio Quartulli

2013-08-02 07:18 UTC #4 lore raw

Re: [B.A.T.M.A.N.] [PATCH next 2/2] batman-adv: fix potential kernel paging error for unicast transmissions

Quoted 12 lines · Marek Lindner

Quoted 11 lines · On Monday, July 29, 2013 04:17:57 Antonio Quartulli wrote:
Il 27.07.2013 03:24 Linus Lüssing ha scritto:
Quoted 6 lines
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!

Quoted 8 lines

Quoted 5 lines
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,

Signature · 4 lines

Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

Linus Lüssing

author

2013-08-02 08:04 UTC #5 lore raw

Re: [B.A.T.M.A.N.] [PATCH next 2/2] batman-adv: fix potential kernel paging error for unicast transmissions

Quoted 22 lines · Antonio Quartulli

Quoted 12 lines · Marek Lindner
Quoted 11 lines · On Monday, July 29, 2013 04:17:57 Antonio Quartulli wrote:
Il 27.07.2013 03:24 Linus Lüssing ha scritto:
Quoted 6 lines
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!
Quoted 8 lines
Quoted 5 lines
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 ?

Quoted 6 lines

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).

Quoted 8 lines

Cheers,

Signature · 4 lines

Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

Antonio Quartulli

Acked-by on v1 1/2

2013-07-28 20:14 UTC #6 lore raw

In reply to Linus Lüssing

Il 27.07.2013 03:24 Linus Lüssing ha scritto:

Quoted 1 line

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.

Quoted 5 lines

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 <[email protected]>

Acked-by Antonio Quartulli <[email protected]>

Signature · 1 line

Antonio Quartulli

Marek Lindner

Applied

2013-08-02 04:02 UTC #7 lore raw

Quoted 7 lines · On Monday, July 29, 2013 04:14:23 Antonio Quartulli wrote:

Il 27.07.2013 03:24 Linus Lüssing ha scritto:
Quoted 2 lines
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.

Quoted 7 lines

Quoted 5 lines
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 <[email protected]>
Acked-by Antonio Quartulli <[email protected]>

Applied in revision d781a70.

Thanks,
Marek