Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752144AbdFAQoi (ORCPT ); Thu, 1 Jun 2017 12:44:38 -0400 Received: from narfation.org ([79.140.41.39]:50576 "EHLO v3-1039.vlinux.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751742AbdFAQog (ORCPT ); Thu, 1 Jun 2017 12:44:36 -0400 From: Sven Eckelmann To: Ben Hutchings Cc: linux-kernel@vger.kernel.org, stable@vger.kernel.org, akpm@linux-foundation.org, Simon Wunderlich Subject: Re: [PATCH 3.16 144/212] batman-adv: Fix double free during fragment merge error Date: Thu, 01 Jun 2017 18:44:29 +0200 Message-ID: <2107568.RkcaoAHtPv@sven-edge> User-Agent: KMail/5.2.3 (Linux/4.9.0-3-amd64; KDE/5.28.0; x86_64; ; ) In-Reply-To: References: MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart24533107.CAxLhfCRhp"; micalg="pgp-sha512"; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3860 Lines: 89 --nextPart24533107.CAxLhfCRhp Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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 > > 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 > Signed-off-by: Simon Wunderlich > [bwh: Backported to 3.16: adjust context] > Signed-off-by: Ben Hutchings > --- > 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 --nextPart24533107.CAxLhfCRhp Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEF10rh2Elc9zjMuACXYcKB8Eme0YFAlkwRG0ACgkQXYcKB8Em e0b7gQ//X9JLmuVnK/krFd9voRPPOEwmukIfJTb80NrHh7Ie96VjYz/cEVHrlBv/ flb2HtqWEVFA3SrsS/H/lO+BBzcZuELj47SwRblpFB9FDXoYw6X48RErGWcz6wA0 biazyZQgSZjiQXueaLykY3/sJXZHD88eOdq1uSsI8citxJClbx9BAHt+xteuWd6O bKQetyMvp/RnunXhNwBISBsgjjBOxsX/MuPAoSyA5yl+cZGPh1KV+JKmruYJhYah v8bSS/KQCsnAiWQtf7FjA8ElXollhRUemHmsHq2GsQ72fX/dn1nxOGJsHYcITt9i n8fd/g/b7MQQUdEPs97tlq76OslyZvQs/q7n7w8MPNsMXV2c55nXeK9LreVSDVby z03DY5Ni5SBZte/bVaXzj9IO+W13ceUh48gUaMqVgTPsCw+yDVhmNP9Lit+uAtSP jJVVvr9RoY8HVkPcAfadMYhQNz/1BMnNj44hc8TOhRZ3xJB4YzTVEIk1LltAFwJU RIxdO/gYp4vxYJ+zn7mrZqOOvaNjZb141hKLboGoWacWhk9naZtQe8S1JYSKb8Vj rl3dpz1Nz57/mDewNWlPRhMu7EmxSMnW/hLXABK+zrPXAQVj+eMNQhgdvD2O79MX v5J7SFGjrcavzLBvJTpyPN/Sq+OEAJajYirDNQV2tauwVpuBK7U= =gKiF -----END PGP SIGNATURE----- --nextPart24533107.CAxLhfCRhp--