[next,2/2] batman-adv: fix potential kernel paging error for unicast transmissions
Commit Message
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(-)
Comments
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
@@ -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;