[B.A.T.M.A.N.] [PATCH] batman-adv: fix skb->data assignment

[B.A.T.M.A.N.] [PATCH] batman-adv: fix skb->data assignment

[Antonio Quartulli]

skb_linearize(skb) possibly rearranges the skb internal data and then changes
the skb->data pointer value. For this reason any other pointer in the code that
was assigned skb->data before invoking skb_linearise(skb) must be re-assigned.

In the current tt_query message handling code this is not done and therefore, in
case of skb linearization, the pointer used to handle the packet header ends up
in pointing to poisoned memory. The packet is then dropped but the
translation-table mechanism is corrupted.

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---

*** this patch is an important fix and it is for maint ***

 routing.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/routing.c b/routing.c
index be068f3..b79e42e 100644
--- a/routing.c
+++ b/routing.c
@@ -638,6 +638,8 @@ int batadv_recv_tt_query(struct sk_buff *skb, struct batadv_hard_iface *recv_if)
 			 */
 			if (skb_linearize(skb) < 0)
 				goto out;
+			/* skb_linearize() possibly changed skb->data */
+			tt_query = (struct batadv_tt_query_packet *)skb->data;
 
 			tt_size = batadv_tt_len(ntohs(tt_query->tt_data));
 
-- 
1.7.9.4

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: fix skb->data assignment

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]

On Thursday 14 June 2012 22:21:28 Antonio Quartulli wrote:
> skb_linearize(skb) possibly rearranges the skb internal data and then
> changes the skb->data pointer value. For this reason any other pointer in
> the code that was assigned skb->data before invoking skb_linearise(skb)
> must be re-assigned.
> 
> In the current tt_query message handling code this is not done and
> therefore, in case of skb linearization, the pointer used to handle the
> packet header ends up in pointing to poisoned memory. The packet is then
> dropped but the translation-table mechanism is corrupted.
> 
> Signed-off-by: Antonio Quartulli <ordex@autistici.org>
> ---
> 
> *** this patch is an important fix and it is for maint ***

Don't forget to add 

Cc: stable <stable@vger.kernel.org>

to the patch and a small explanation since when the bug is there (I guess 
v3.1) and that it may lead to crashes and not only poisened memory (that is 
the best case.. but maybe the page was removed and we end up in hell when 
accessing the memory region).

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: fix skb->data assignment

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

On Fri, Jun 15, 2012 at 01:45:11PM +0200, Sven Eckelmann wrote:
> On Thursday 14 June 2012 22:21:28 Antonio Quartulli wrote:
> > skb_linearize(skb) possibly rearranges the skb internal data and then
> > changes the skb->data pointer value. For this reason any other pointer in
> > the code that was assigned skb->data before invoking skb_linearise(skb)
> > must be re-assigned.
> > 
> > In the current tt_query message handling code this is not done and
> > therefore, in case of skb linearization, the pointer used to handle the
> > packet header ends up in pointing to poisoned memory. The packet is then
> > dropped but the translation-table mechanism is corrupted.
> > 
> > Signed-off-by: Antonio Quartulli <ordex@autistici.org>
> > ---
> > 
> > *** this patch is an important fix and it is for maint ***
> 
> Don't forget to add 
> 
> Cc: stable <stable@vger.kernel.org>
> 
> to the patch and a small explanation since when the bug is there (I guess 
> v3.1) and that it may lead to crashes and not only poisened memory (that is 
> the best case.. but maybe the page was removed and we end up in hell when 
> accessing the memory region).

Hi Sven,

Thank you for your suggestions. Sure, I will.

Regards,


-- 
Antonio Quartulli

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

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: fix skb->data assignment

[Marek Lindner]

On Friday, June 15, 2012 04:21:28 Antonio Quartulli wrote:
> skb_linearize(skb) possibly rearranges the skb internal data and then
> changes the skb->data pointer value. For this reason any other pointer in
> the code that was assigned skb->data before invoking skb_linearise(skb)
> must be re-assigned.
> 
> In the current tt_query message handling code this is not done and
> therefore, in case of skb linearization, the pointer used to handle the
> packet header ends up in pointing to poisoned memory. The packet is then
> dropped but the translation-table mechanism is corrupted.
> 
> Signed-off-by: Antonio Quartulli <ordex@autistici.org>
> ---
> 
> *** this patch is an important fix and it is for maint ***

Next time you send a patch for maint, please be sure the patch is actually 
based on maint. Your patch does not even apply on top of maint ...

Applied in revision c7d05ee.

Thanks,
Marek

[B.A.T.M.A.N.] [PATCH] batman-adv: fix skb->data assignment

[Antonio Quartulli]

commit d2b6cc8e460494251442a877fcbc150faa175b4f upstream.

skb_linearize(skb) possibly rearranges the skb internal data and then changes
the skb->data pointer value. For this reason any other pointer in the code that
was assigned skb->data before invoking skb_linearise(skb) must be re-assigned.

In the current tt_query message handling code this is not done and therefore, in
case of skb linearization, the pointer used to handle the packet header ends up
in pointing to poisoned memory. The packet is then dropped but the
translation-table mechanism is corrupted.

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
Cc: <stable@vger.kernel.org> # 3.3
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 net/batman-adv/routing.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
index 773e606..7d3ac4e 100644
--- a/net/batman-adv/routing.c
+++ b/net/batman-adv/routing.c
@@ -622,6 +622,8 @@ int recv_tt_query(struct sk_buff *skb, struct hard_iface *recv_if)
 			 * changes */
 			if (skb_linearize(skb) < 0)
 				goto out;
+			/* skb_linearize() possibly changed skb->data */
+			tt_query = (struct tt_query_packet *)skb->data;
 
 			tt_len = tt_query->tt_data * sizeof(struct tt_change);
 
-- 
1.7.9.4

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: fix skb->data assignment

[David Miller]

From: Antonio Quartulli <ordex@autistici.org>
Date: Tue, 19 Jun 2012 09:20:30 +0200

> commit d2b6cc8e460494251442a877fcbc150faa175b4f upstream.

That commit does not exist in Linus's tree.

It exists in the net-next tree.

You must only make stable submissions for patches that have
made their way into Linus's tree.

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: fix skb->data assignment

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

On Tuesday 19 June 2012 00:41:40 David Miller wrote:
[....]
> You must only make stable submissions for patches that have
> made their way into Linus's tree.

That is correct. These patches were meant to be sent as reply to the patch you 
pulled today (that contained the initial Cc: stable@...). These were sent to 
avoid the cherry-pick of feature patches necessary to merge it and still give 
an idea how these versions could be fixed after the mentioned commit hit 
Linus' tree.

Somebody forgot the set the In-Reply-To:.. that made these mails missing the 
right context.

But we were wondering how to handle this situation anyway. The 
stable_kernel_rules.txt didn't gave us the correct information (or we did not 
understand them correctly) how to submit backported patches for affected 
kernels. Are there some general rules that we should be aware of?

Thanks,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: fix skb->data assignment

[David Miller]

From: Sven Eckelmann <sven@narfation.org>
Date: Tue, 19 Jun 2012 10:07:12 +0200

> On Tuesday 19 June 2012 00:41:40 David Miller wrote:
> [....]
>> You must only make stable submissions for patches that have
>> made their way into Linus's tree.
> 
> That is correct. These patches were meant to be sent as reply to the patch you 
> pulled today (that contained the initial Cc: stable@...). These were sent to 
> avoid the cherry-pick of feature patches necessary to merge it and still give 
> an idea how these versions could be fixed after the mentioned commit hit 
> Linus' tree.

Patches you want to end up in -stable should be submitted for 'net'
not 'net-next'.

There is no other valid submission scheme.

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: fix skb->data assignment

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 1402 bytes --]

On Tuesday 19 June 2012 02:02:27 David Miller wrote:
[....]
> Patches you want to end up in -stable should be submitted for 'net'
> not 'net-next'.
> 
> There is no other valid submission scheme.

I know that. This is also completely right and not contested by me. I did not 
submit/create the pull request and don't want to talk about that part too 
much. At least I am not very good in speaking for other people and don't want 
to create more hiccups than I already caused :)

The part that was more interesting for me is the backporting (+ submission of 
these patches) or annotation to which kernel that patch should be backported. 
The stable_kernel_rules.txt gives a nice overview about how to say which other 
patches should be cherry-picked to get this fix applied. But we could also 
have the problem with kernel version that are not obvious (since the patch 
doesn't apply directly on that version) and a backported patch is a better 
choice than cherry-picking many other things.

Is it correct as Antonio did it (submitting more than one patch, but with a 
special "# 3.x" for each one after the Cc: )?

I don't want a big discussion. Just a small hint how it is preferred (maybe 
nothing should be done and the stable committee will guess it.. for example 
the commit id that introduced the problem was missing, but could be used to 
find the affected kernels quite easily).

Thanks,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: fix skb->data assignment

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1905 bytes --]

On Tue, 2012-06-19 at 11:51 +0200, Sven Eckelmann wrote:
> On Tuesday 19 June 2012 02:02:27 David Miller wrote:
> [....]
> > Patches you want to end up in -stable should be submitted for 'net'
> > not 'net-next'.
> > 
> > There is no other valid submission scheme.
> 
> I know that. This is also completely right and not contested by me. I did not 
> submit/create the pull request and don't want to talk about that part too 
> much. At least I am not very good in speaking for other people and don't want 
> to create more hiccups than I already caused :)
> 
> The part that was more interesting for me is the backporting (+ submission of 
> these patches) or annotation to which kernel that patch should be backported. 
> The stable_kernel_rules.txt gives a nice overview about how to say which other 
> patches should be cherry-picked to get this fix applied. But we could also 
> have the problem with kernel version that are not obvious (since the patch 
> doesn't apply directly on that version) and a backported patch is a better 
> choice than cherry-picking many other things.
> 
> Is it correct as Antonio did it (submitting more than one patch, but with a 
> special "# 3.x" for each one after the Cc: )?
[...]

There's not much sense in a 'cc' if you're sending a new message (the
backport) specificaly to stable.  Instead, use a subject like
'[PATCH 3.x] original commit summary'.  You do need to make clear that
this is a backport and not just a request to cherry-pick the upstream
commit.

Also, you should wait and send it after the original goes upstream,
because stable maintainers will usually want to either apply or reject
immediately rather than storing things up.  Greg's scripts will tell you
when this happens, or at least when he next checks.

Ben.

-- 
Ben Hutchings
If more than one person is responsible for a bug, no one is at fault.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [B.A.T.M.A.N.] [PATCH] batman-adv: fix skb->data assignment

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]

On Tuesday 19 June 2012 13:10:11 Ben Hutchings wrote:
> [...]
> 
> There's not much sense in a 'cc' if you're sending a new message (the
> backport) specificaly to stable.  Instead, use a subject like
> '[PATCH 3.x] original commit summary'.  You do need to make clear that
> this is a backport and not just a request to cherry-pick the upstream
> commit.
> 
> Also, you should wait and send it after the original goes upstream,
> because stable maintainers will usually want to either apply or reject
> immediately rather than storing things up.  Greg's scripts will tell you
> when this happens, or at least when he next checks.

Thanks a lot.

Regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[B.A.T.M.A.N.] [PATCH] batman-adv: fix skb->data assignment

[Antonio Quartulli]

commit d2b6cc8e460494251442a877fcbc150faa175b4f upstream.

skb_linearize(skb) possibly rearranges the skb internal data and then changes
the skb->data pointer value. For this reason any other pointer in the code that
was assigned skb->data before invoking skb_linearise(skb) must be re-assigned.

In the current tt_query message handling code this is not done and therefore, in
case of skb linearization, the pointer used to handle the packet header ends up
in pointing to poisoned memory. The packet is then dropped but the
translation-table mechanism is corrupted.

Signed-off-by: Antonio Quartulli <ordex@autistici.org>
Cc: <stable@vger.kernel.org> # 3.1
Cc: <stable@vger.kernel.org> # 3.2
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
 net/batman-adv/routing.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
index 0f32c81..55136e5 100644
--- a/net/batman-adv/routing.c
+++ b/net/batman-adv/routing.c
@@ -1246,6 +1246,8 @@ int recv_tt_query(struct sk_buff *skb, struct hard_iface *recv_if)
 		/* packet needs to be linearised to access the TT changes */
 		if (skb_linearize(skb) < 0)
 			goto out;
+		/* skb_linearize() possibly changed skb->data */
+		tt_query = (struct tt_query_packet *)skb->data;
 
 		if (is_my_mac(tt_query->dst))
 			handle_tt_response(bat_priv, tt_query);
-- 
1.7.9.4