[PATCH] mac80211: Fix regression that triggers a kernel BUG with CCMP

[PATCH] mac80211: Fix regression that triggers a kernel BUG with CCMP

[Ronald Wahl]

Commit 7ec7c4a9a686c608315739ab6a2b0527a240883c (mac80211: port CCMP to
cryptoapi's CCM driver) introduced a regression when decrypting empty
packets (data_len == 0). This will lead to backtraces like:

(scatterwalk_start) from [<c01312f4>] (scatterwalk_map_and_copy+0x2c/0xa8)
(scatterwalk_map_and_copy) from [<c013a5a0>] (crypto_ccm_decrypt+0x7c/0x25c)
(crypto_ccm_decrypt) from [<c032886c>] (ieee80211_aes_ccm_decrypt+0x160/0x170)
(ieee80211_aes_ccm_decrypt) from [<c031c628>] (ieee80211_crypto_ccmp_decrypt+0x1ac/0x238)
(ieee80211_crypto_ccmp_decrypt) from [<c032ef28>] (ieee80211_rx_handlers+0x870/0x1d24)
(ieee80211_rx_handlers) from [<c0330c7c>] (ieee80211_prepare_and_rx_handle+0x8a0/0x91c)
(ieee80211_prepare_and_rx_handle) from [<c0331260>] (ieee80211_rx+0x568/0x730)
(ieee80211_rx) from [<c01d3054>] (__carl9170_rx+0x94c/0xa20)
(__carl9170_rx) from [<c01d3324>] (carl9170_rx_stream+0x1fc/0x320)
(carl9170_rx_stream) from [<c01cbccc>] (carl9170_usb_tasklet+0x80/0xc8)
(carl9170_usb_tasklet) from [<c00199dc>] (tasklet_hi_action+0x88/0xcc)
(tasklet_hi_action) from [<c00193c8>] (__do_softirq+0xcc/0x200)
(__do_softirq) from [<c0019734>] (irq_exit+0x80/0xe0)
(irq_exit) from [<c0009c10>] (handle_IRQ+0x64/0x80)
(handle_IRQ) from [<c000c3a0>] (__irq_svc+0x40/0x4c)
(__irq_svc) from [<c0009d44>] (arch_cpu_idle+0x2c/0x34)

Such packets can appear for example when using the carl9170 wireless driver
because hardware sometimes generates garbage when the internal FIFO overruns.

This patch adds an additional length check.

Signed-off-by: Ronald Wahl <ronald.wahl@raritan.com>
---
 net/mac80211/aes_ccm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
index ec24378..0cd9523 100644
--- a/net/mac80211/aes_ccm.c
+++ b/net/mac80211/aes_ccm.c
@@ -53,6 +53,9 @@ int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
 		__aligned(__alignof__(struct aead_request));
 	struct aead_request *aead_req = (void *) aead_req_data;
 
+	if (data_len == 0)
+		return EINVAL;
+
 	memset(aead_req, 0, sizeof(aead_req_data));
 
 	sg_init_one(&pt, data, data_len);
-- 
1.9.3

Re: [PATCH] mac80211: Fix regression that triggers a kernel BUG with CCMP

[Ard Biesheuvel]

On 6 November 2014 10:55, Ronald Wahl <ronald.wahl@raritan.com> wrote:
> Commit 7ec7c4a9a686c608315739ab6a2b0527a240883c (mac80211: port CCMP to
> cryptoapi's CCM driver) introduced a regression when decrypting empty
> packets (data_len == 0). This will lead to backtraces like:
>
> (scatterwalk_start) from [<c01312f4>] (scatterwalk_map_and_copy+0x2c/0xa8)
> (scatterwalk_map_and_copy) from [<c013a5a0>] (crypto_ccm_decrypt+0x7c/0x25c)
> (crypto_ccm_decrypt) from [<c032886c>] (ieee80211_aes_ccm_decrypt+0x160/0x170)
> (ieee80211_aes_ccm_decrypt) from [<c031c628>] (ieee80211_crypto_ccmp_decrypt+0x1ac/0x238)
> (ieee80211_crypto_ccmp_decrypt) from [<c032ef28>] (ieee80211_rx_handlers+0x870/0x1d24)
> (ieee80211_rx_handlers) from [<c0330c7c>] (ieee80211_prepare_and_rx_handle+0x8a0/0x91c)
> (ieee80211_prepare_and_rx_handle) from [<c0331260>] (ieee80211_rx+0x568/0x730)
> (ieee80211_rx) from [<c01d3054>] (__carl9170_rx+0x94c/0xa20)
> (__carl9170_rx) from [<c01d3324>] (carl9170_rx_stream+0x1fc/0x320)
> (carl9170_rx_stream) from [<c01cbccc>] (carl9170_usb_tasklet+0x80/0xc8)
> (carl9170_usb_tasklet) from [<c00199dc>] (tasklet_hi_action+0x88/0xcc)
> (tasklet_hi_action) from [<c00193c8>] (__do_softirq+0xcc/0x200)
> (__do_softirq) from [<c0019734>] (irq_exit+0x80/0xe0)
> (irq_exit) from [<c0009c10>] (handle_IRQ+0x64/0x80)
> (handle_IRQ) from [<c000c3a0>] (__irq_svc+0x40/0x4c)
> (__irq_svc) from [<c0009d44>] (arch_cpu_idle+0x2c/0x34)
>
> Such packets can appear for example when using the carl9170 wireless driver
> because hardware sometimes generates garbage when the internal FIFO overruns.
>
> This patch adds an additional length check.
>
> Signed-off-by: Ronald Wahl <ronald.wahl@raritan.com>
> ---
>  net/mac80211/aes_ccm.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/mac80211/aes_ccm.c b/net/mac80211/aes_ccm.c
> index ec24378..0cd9523 100644
> --- a/net/mac80211/aes_ccm.c
> +++ b/net/mac80211/aes_ccm.c
> @@ -53,6 +53,9 @@ int ieee80211_aes_ccm_decrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad,
>                 __aligned(__alignof__(struct aead_request));
>         struct aead_request *aead_req = (void *) aead_req_data;
>
> +       if (data_len == 0)
> +               return EINVAL;
> +

Did you mean -EINVAL?

-- 
Ard.

>         memset(aead_req, 0, sizeof(aead_req_data));
>
>         sg_init_one(&pt, data, data_len);
> --
> 1.9.3
>

Re: [PATCH] mac80211: Fix regression that triggers a kernel BUG with CCMP

[Johannes Berg]

On Thu, 2014-11-06 at 10:55 +0100, Ronald Wahl wrote:
> Commit 7ec7c4a9a686c608315739ab6a2b0527a240883c (mac80211: port CCMP to
> cryptoapi's CCM driver) introduced a regression when decrypting empty
> packets (data_len == 0). This will lead to backtraces like:

Not good.

> This patch adds an additional length check.
> 
> Signed-off-by: Ronald Wahl <ronald.wahl@raritan.com>

Please add

Cc: stable@vger.kernel.org
Fixes: 7ec7c4a9a686 ("mac80211: port CCMP to cryptoapi's CCM driver")

> +	if (data_len == 0)
> +		return EINVAL;

error codes should be negative. Or we could just return 0 to pretend it
was decrypted, after all, there was nothing to do? The packet is useless
though, so might as well drop it (and we would later anyway)

Oh, regarding Cc stable - don't actually send the mail there if you can
help it.

johannes

Re: [PATCH] mac80211: Fix regression that triggers a kernel BUG with CCMP

[Ronald Wahl]

On 06.11.2014 11:02, Johannes Berg wrote:
> On Thu, 2014-11-06 at 10:55 +0100, Ronald Wahl wrote:
>> Commit 7ec7c4a9a686c608315739ab6a2b0527a240883c (mac80211: port CCMP to
>> cryptoapi's CCM driver) introduced a regression when decrypting empty
>> packets (data_len == 0). This will lead to backtraces like:
>
> Not good.
>
>> This patch adds an additional length check.
>>
>> Signed-off-by: Ronald Wahl <ronald.wahl@raritan.com>
>
> Please add
>
> Cc: stable@vger.kernel.org
> Fixes: 7ec7c4a9a686 ("mac80211: port CCMP to cryptoapi's CCM driver")
>
>> +	if (data_len == 0)
>> +		return EINVAL;
>
> error codes should be negative.

Argh. New patch on the way...

thx,
ron

-- 
Ronald Wahl - ronald.wahl@raritan.com - Phone +49 375271349-0 Fax -99
Raritan Deutschland GmbH, Kornmarkt 7, 08056 Zwickau, Germany
USt-IdNr. DE813094160, Steuer-Nr. 227/117/01749
Amtsgericht Chemnitz HRB 23605
Geschäftsführung: Stuart Hopper, Ralf Ploenes

Re: [PATCH] mac80211: Fix regression that triggers a kernel BUG with CCMP

[Arend van Spriel]

On 06-11-14 11:02, Johannes Berg wrote:
> Oh, regarding Cc stable - don't actually send the mail there if you can
> help it.

What do you mean? Are you saying: It must be tagged, but 'git
send-email' should not actually add the Cc field in the mail header.
What is the story here? Guess I missed a memo ;-)

> johannes

Regards,
Arend

Re: [PATCH] mac80211: Fix regression that triggers a kernel BUG with CCMP

[Johannes Berg]

On Thu, 2014-11-06 at 13:34 +0100, Arend van Spriel wrote:
> On 06-11-14 11:02, Johannes Berg wrote:
> > Oh, regarding Cc stable - don't actually send the mail there if you can
> > help it.
> 
> What do you mean? Are you saying: It must be tagged, but 'git
> send-email' should not actually add the Cc field in the mail header.
> What is the story here? Guess I missed a memo ;-)

As far as I remember Greg just ignores the patches sent to the list and
picks them up when they hit Linus's tree instead (i.e. follows the
commit logs with like 'git log --grep "Cc: stable"') so there's no value
in actually sending out email, it's just noise.

So far it has worked for me, so I think it must be right :)

johannes

Re: [PATCH] mac80211: Fix regression that triggers a kernel BUG with CCMP

[Ronald Wahl]

On 06.11.2014 13:39, Johannes Berg wrote:
> On Thu, 2014-11-06 at 13:34 +0100, Arend van Spriel wrote:
>> On 06-11-14 11:02, Johannes Berg wrote:
>>> Oh, regarding Cc stable - don't actually send the mail there if you can
>>> help it.
>>
>> What do you mean? Are you saying: It must be tagged, but 'git
>> send-email' should not actually add the Cc field in the mail header.
>> What is the story here? Guess I missed a memo ;-)
>
> As far as I remember Greg just ignores the patches sent to the list and
> picks them up when they hit Linus's tree instead (i.e. follows the
> commit logs with like 'git log --grep "Cc: stable"') so there's no value
> in actually sending out email, it's just noise.
>
> So far it has worked for me, so I think it must be right :)

But there are LTS kernels not maintained by Greg like 3.12 and 3.16. How 
about these? If sending patches to stable@kernel org is optional then 
it's better to kill that list or silently drop emails send to this list 
so no errors are returned for this address.

- ron

-- 
Ronald Wahl - ronald.wahl@raritan.com - Phone +49 375271349-0 Fax -99
Raritan Deutschland GmbH, Kornmarkt 7, 08056 Zwickau, Germany
USt-IdNr. DE813094160, Steuer-Nr. 227/117/01749
Amtsgericht Chemnitz HRB 23605
Geschäftsführung: Stuart Hopper, Ralf Ploenes

Re: [PATCH] mac80211: Fix regression that triggers a kernel BUG with CCMP

[Johannes Berg]

On Thu, 2014-11-06 at 14:07 +0100, Ronald Wahl wrote:

> But there are LTS kernels not maintained by Greg like 3.12 and 3.16. How 
> about these? If sending patches to stable@kernel org is optional then 
> it's better to kill that list or silently drop emails send to this list 
> so no errors are returned for this address.

They can pick it up the same way though, no? Actually I think they
usually pick it up from Greg's tree anyway :)

johannes

Re: [PATCH] mac80211: Fix regression that triggers a kernel BUG with CCMP

[Ronald Wahl]

On 06.11.2014 14:14, Johannes Berg wrote:
> On Thu, 2014-11-06 at 14:07 +0100, Ronald Wahl wrote:
>
>> But there are LTS kernels not maintained by Greg like 3.12 and 3.16. How
>> about these? If sending patches to stable@kernel org is optional then
>> it's better to kill that list or silently drop emails send to this list
>> so no errors are returned for this address.
>
> They can pick it up the same way though, no? Actually I think they
> usually pick it up from Greg's tree anyway :)

"Can" and "do" are different things. And you "think" you know what 
others people do but do you really "know" it? Anyway your original 
comment sounded a bit like "avoid sending it to the stable kernel 
mailinglist otherwise unwanted things might happen". In the end this 
raises the question why that stable kernel mailing still list exists or 
why there is no general rule not to send mails to it.

So in the end that currently means that it is not wrong to send mail to 
stable@vger.kernel.org but it is your opinion that it is just not really 
necessary, right?

- ron

-- 
Ronald Wahl - ronald.wahl@raritan.com - Phone +49 375271349-0 Fax -99
Raritan Deutschland GmbH, Kornmarkt 7, 08056 Zwickau, Germany
USt-IdNr. DE813094160, Steuer-Nr. 227/117/01749
Amtsgericht Chemnitz HRB 23605
Geschäftsführung: Stuart Hopper, Ralf Ploenes

Re: [PATCH] mac80211: Fix regression that triggers a kernel BUG with CCMP

[Ard Biesheuvel]

On 6 November 2014 14:50, Ronald Wahl <ronald.wahl@raritan.com> wrote:
> On 06.11.2014 14:14, Johannes Berg wrote:
>>
>> On Thu, 2014-11-06 at 14:07 +0100, Ronald Wahl wrote:
>>
>>> But there are LTS kernels not maintained by Greg like 3.12 and 3.16. How
>>> about these? If sending patches to stable@kernel org is optional then
>>> it's better to kill that list or silently drop emails send to this list
>>> so no errors are returned for this address.
>>
>>
>> They can pick it up the same way though, no? Actually I think they
>> usually pick it up from Greg's tree anyway :)
>
>
> "Can" and "do" are different things. And you "think" you know what others
> people do but do you really "know" it? Anyway your original comment sounded
> a bit like "avoid sending it to the stable kernel mailinglist otherwise
> unwanted things might happen". In the end this raises the question why that
> stable kernel mailing still list exists or why there is no general rule not
> to send mails to it.
>
> So in the end that currently means that it is not wrong to send mail to
> stable@vger.kernel.org but it is your opinion that it is just not really
> necessary, right?
>

You used to get an autoreply like this when cc'ing stable@ directly

"""
<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
for how to do this properly.

</formletter>
"""

You are only supposed to send patches to the mailing list if they are
already upstream, i.e., if you know the upstream commit id of the
patch.

-- 
Ard.

Re: [PATCH] mac80211: Fix regression that triggers a kernel BUG with CCMP

[Arend van Spriel]

On 11/06/14 14:55, Ard Biesheuvel wrote:
> On 6 November 2014 14:50, Ronald Wahl<ronald.wahl@raritan.com>  wrote:
>> On 06.11.2014 14:14, Johannes Berg wrote:
>>>
>>> On Thu, 2014-11-06 at 14:07 +0100, Ronald Wahl wrote:
>>>
>>>> But there are LTS kernels not maintained by Greg like 3.12 and 3.16. How
>>>> about these? If sending patches to stable@kernel org is optional then
>>>> it's better to kill that list or silently drop emails send to this list
>>>> so no errors are returned for this address.
>>>
>>>
>>> They can pick it up the same way though, no? Actually I think they
>>> usually pick it up from Greg's tree anyway :)
>>
>>
>> "Can" and "do" are different things. And you "think" you know what others
>> people do but do you really "know" it? Anyway your original comment sounded
>> a bit like "avoid sending it to the stable kernel mailinglist otherwise
>> unwanted things might happen". In the end this raises the question why that
>> stable kernel mailing still list exists or why there is no general rule not
>> to send mails to it.
>>
>> So in the end that currently means that it is not wrong to send mail to
>> stable@vger.kernel.org but it is your opinion that it is just not really
>> necessary, right?
>>
>
> You used to get an autoreply like this when cc'ing stable@ directly
>
> """
> <formletter>
>
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
> for how to do this properly.
>
> </formletter>
> """
>
> You are only supposed to send patches to the mailing list if they are
> already upstream, i.e., if you know the upstream commit id of the
> patch.

Actually, adding the "Cc: stable@vger.kernel.org" tag will assure that 
Greg picks it up automatically from upstream thus no need to send the 
patch to stable provided it will apply to stable trees as is.

Regards,
Arend