Re: [PATCH 3.16 144/212] batman-adv: Fix double free during fragment merge error

[Sven Eckelmann]

On Donnerstag, 1. Juni 2017 16:43:16 CEST Ben Hutchings wrote:
> 3.16.44-rc1 review patch.  If anyone has any objections, please let me know.

It looks to me like there are problems with this backport. The surrounding 
code has to be adjusted slightly further to avoid additional problems.

> ------------------
> 
> From: Sven Eckelmann <sven@xxxxxxxxxxxxx>
> 
> commit 248e23b50e2da0753f3b5faa068939cbe9f8a75a upstream.
> 
> The function batadv_frag_skb_buffer was supposed not to consume the skbuff
> on errors. This was followed in the helper function
> batadv_frag_insert_packet when the skb would potentially be inserted in the
> fragment queue. But it could happen that the next helper function
> batadv_frag_merge_packets would try to merge the fragments and fail. This
> results in a kfree_skb of all the enqueued fragments (including the just
> inserted one). batadv_recv_frag_packet would detect the error in
> batadv_frag_skb_buffer and try to free the skb again.
> 
> The behavior of batadv_frag_skb_buffer (and its helper
> batadv_frag_insert_packet) must therefore be changed to always consume the
> skbuff to have a common behavior and avoid the double kfree_skb.
> 
> Fixes: 610bfc6bc99b ("batman-adv: Receive fragmented packets and merge")
> Signed-off-by: Sven Eckelmann <sven@xxxxxxxxxxxxx>
> Signed-off-by: Simon Wunderlich <sw@xxxxxxxxxxxxxxxxxx>
> [bwh: Backported to 3.16: adjust context]
> Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
> ---
>  net/batman-adv/fragmentation.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

It is not really easy to see but this change will introduce a new double free 
for kernels older than v4.10. The relevant commit is b91a2543b4c1 ("batman-
adv: Consume skb in receive handlers"). This was discussed in following gluon 
ticket https://github.com/freifunk-gluon/gluon/issues/1083 (just in case you 
are interested about the details)

Following change must therefore be added to this patch on older kernels:

    --- a/net/batman-adv/routing.c
    +++ b/net/batman-adv/routing.c
    @@ -961,6 +961,12 @@ int batadv_recv_frag_packet(struct sk_buff *skb,
     	batadv_inc_counter(bat_priv, BATADV_CNT_FRAG_RX);
     	batadv_add_counter(bat_priv, BATADV_CNT_FRAG_RX_BYTES, skb->len);
     
    +	/* batadv_frag_skb_buffer will always consume the skb and
    +	 * the caller should therefore never try to free the
    +	 * skb after this point
    +	 */
    +	ret = NET_RX_SUCCESS;
    +
     	/* Add fragment to buffer and merge if possible. */
     	if (!batadv_frag_skb_buffer(&skb, orig_node_src))
     		goto out;

You can also remove the same instruction which appears later in this function.

Kind regards,
	Sven

Attachment: signature.asc
Description: This is a digitally signed message part.