[PATCH] ath6kl: protect firmware from excessive WoW pattern length

[PATCH] ath6kl: protect firmware from excessive WoW pattern length

[Thomas Pedersen]

Don't accept WoW patterns longer than supported by firmware.

Reported-by: Haijun Jin <nhjin@qca.qualcomm.com>
Signed-off-by: Thomas Pedersen <c_tpeder@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath6kl/cfg80211.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c b/drivers/net/wireless/ath/ath6kl/cfg80211.c
index bd003fe..ffa18f3 100644
--- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
+++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
@@ -1876,6 +1876,9 @@ static int ath6kl_wow_usr(struct ath6kl *ar, struct ath6kl_vif *vif,
 	/* Configure the patterns that we received from the user. */
 	for (i = 0; i < wow->n_patterns; i++) {
 
+		if (wow->patterns[i].pattern_len > WOW_MASK_SIZE)
+			return -EINVAL;
+
 		/*
 		 * Convert given nl80211 specific mask value to equivalent
 		 * driver specific mask value and send it to the chip along
-- 
1.7.4.1

Re: [PATCH] ath6kl: protect firmware from excessive WoW pattern length

[Kalle Valo]

On 08/16/2012 03:15 AM, Thomas Pedersen wrote:
> Don't accept WoW patterns longer than supported by firmware.
> 
> Reported-by: Haijun Jin <nhjin@qca.qualcomm.com>
> Signed-off-by: Thomas Pedersen <c_tpeder@qca.qualcomm.com>

Thanks, applied.

Kalle

Re: [PATCH] ath6kl: protect firmware from excessive WoW pattern length

[Johannes Berg]

On Wed, 2012-08-15 at 17:15 -0700, Thomas Pedersen wrote:
> Don't accept WoW patterns longer than supported by firmware.
> 
> Reported-by: Haijun Jin <nhjin@qca.qualcomm.com>
> Signed-off-by: Thomas Pedersen <c_tpeder@qca.qualcomm.com>
> ---
>  drivers/net/wireless/ath/ath6kl/cfg80211.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c b/drivers/net/wireless/ath/ath6kl/cfg80211.c
> index bd003fe..ffa18f3 100644
> --- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
> +++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
> @@ -1876,6 +1876,9 @@ static int ath6kl_wow_usr(struct ath6kl *ar, struct ath6kl_vif *vif,
>  	/* Configure the patterns that we received from the user. */
>  	for (i = 0; i < wow->n_patterns; i++) {
>  
> +		if (wow->patterns[i].pattern_len > WOW_MASK_SIZE)
> +			return -EINVAL;
> +

No objection, but doesn't nl80211 already validate that (assuming you
give the right pattern_max_len, of course)?

johannes

Re: [PATCH] ath6kl: protect firmware from excessive WoW pattern length

[Kalle Valo]

On 08/20/2012 10:13 AM, Johannes Berg wrote:
> On Wed, 2012-08-15 at 17:15 -0700, Thomas Pedersen wrote:
>> Don't accept WoW patterns longer than supported by firmware.
>>
>> Reported-by: Haijun Jin <nhjin@qca.qualcomm.com>
>> Signed-off-by: Thomas Pedersen <c_tpeder@qca.qualcomm.com>
>> ---
>>  drivers/net/wireless/ath/ath6kl/cfg80211.c |    3 +++
>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c b/drivers/net/wireless/ath/ath6kl/cfg80211.c
>> index bd003fe..ffa18f3 100644
>> --- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
>> +++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
>> @@ -1876,6 +1876,9 @@ static int ath6kl_wow_usr(struct ath6kl *ar, struct ath6kl_vif *vif,
>>  	/* Configure the patterns that we received from the user. */
>>  	for (i = 0; i < wow->n_patterns; i++) {
>>  
>> +		if (wow->patterns[i].pattern_len > WOW_MASK_SIZE)
>> +			return -EINVAL;
>> +
> 
> No objection, but doesn't nl80211 already validate that (assuming you
> give the right pattern_max_len, of course)?

And ath6kl even uses different define pattern_max_len:

	wiphy->wowlan.pattern_max_len = WOW_PATTERN_SIZE;

But the value is still same:

#define WOW_PATTERN_SIZE	 64
#define WOW_MASK_SIZE		 64

Thomas, can you please check this? Do we really need two different
defines? And which one is the correct one here?

I'll keep the patch applied but I'm happy to take followup patches to
clarify this part.

Kalle

Re: [PATCH] ath6kl: protect firmware from excessive WoW pattern length

[Pedersen, Thomas]

On Mon, Aug 20, 2012 at 10:29:19AM +0300, Kalle Valo wrote:
> On 08/20/2012 10:13 AM, Johannes Berg wrote:
> > On Wed, 2012-08-15 at 17:15 -0700, Thomas Pedersen wrote:
> >> Don't accept WoW patterns longer than supported by firmware.
> >>
> >> Reported-by: Haijun Jin <nhjin@qca.qualcomm.com>
> >> Signed-off-by: Thomas Pedersen <c_tpeder@qca.qualcomm.com>
> >> ---
> >>  drivers/net/wireless/ath/ath6kl/cfg80211.c |    3 +++
> >>  1 files changed, 3 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c b/drivers/net/wireless/ath/ath6kl/cfg80211.c
> >> index bd003fe..ffa18f3 100644
> >> --- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
> >> +++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
> >> @@ -1876,6 +1876,9 @@ static int ath6kl_wow_usr(struct ath6kl *ar, struct ath6kl_vif *vif,
> >>  	/* Configure the patterns that we received from the user. */
> >>  	for (i = 0; i < wow->n_patterns; i++) {
> >>  
> >> +		if (wow->patterns[i].pattern_len > WOW_MASK_SIZE)
> >> +			return -EINVAL;
> >> +
> > 
> > No objection, but doesn't nl80211 already validate that (assuming you
> > give the right pattern_max_len, of course)?

Thanks for pointing that out. That check would be completely redundant
then.

Kalle,

Can you revert this patch? Otherwise the followup will just do the same.

> And ath6kl even uses different define pattern_max_len:
> 
> 	wiphy->wowlan.pattern_max_len = WOW_PATTERN_SIZE;
> 
> But the value is still same:
> 
> #define WOW_PATTERN_SIZE	 64
> #define WOW_MASK_SIZE		 64
> 
> Thomas, can you please check this? Do we really need two different
> defines? And which one is the correct one here?

No AFAICT there is no reason to have two different defines. I can submit
a small patch consolidating these, but it would remove the above hunk
anyway so I need to know whether you'll revert or not.

Thanks,
Thomas

Re: [PATCH] ath6kl: protect firmware from excessive WoW pattern length

[Kalle Valo]

On 08/20/2012 09:18 PM, Pedersen, Thomas wrote:
> On Mon, Aug 20, 2012 at 10:29:19AM +0300, Kalle Valo wrote:
>> On 08/20/2012 10:13 AM, Johannes Berg wrote:
>>
>>> No objection, but doesn't nl80211 already validate that (assuming you
>>> give the right pattern_max_len, of course)?
> 
> Thanks for pointing that out. That check would be completely redundant
> then.
> 
> Kalle,
> 
> Can you revert this patch? Otherwise the followup will just do the same.

I can revert the patch. But IMHO the check isn't that bad, and even
cfg80211 can be buggy sometimes ;)

>> And ath6kl even uses different define pattern_max_len:
>>
>> 	wiphy->wowlan.pattern_max_len = WOW_PATTERN_SIZE;
>>
>> But the value is still same:
>>
>> #define WOW_PATTERN_SIZE	 64
>> #define WOW_MASK_SIZE		 64
>>
>> Thomas, can you please check this? Do we really need two different
>> defines? And which one is the correct one here?
> 
> No AFAICT there is no reason to have two different defines. I can submit
> a small patch consolidating these, but it would remove the above hunk
> anyway so I need to know whether you'll revert or not.

Thanks. I'll revert the patch so please prepare your patch without the
check.

Kalle

Re: [PATCH] ath6kl: protect firmware from excessive WoW pattern length

[Pedersen, Thomas]

On Mon, Aug 20, 2012 at 10:08:47PM +0300, Kalle Valo wrote:
> > Can you revert this patch? Otherwise the followup will just do the same.
> 
> I can revert the patch. But IMHO the check isn't that bad, and even
> cfg80211 can be buggy sometimes ;)
> 

Well it's probably better not to cover any cfg80211 bugs up in the
driver anyway.

> >> And ath6kl even uses different define pattern_max_len:
> >>
> >> 	wiphy->wowlan.pattern_max_len = WOW_PATTERN_SIZE;
> >>
> >> But the value is still same:
> >>
> >> #define WOW_PATTERN_SIZE	 64
> >> #define WOW_MASK_SIZE		 64
> >>
> >> Thomas, can you please check this? Do we really need two different
> >> defines? And which one is the correct one here?
> > 
> > No AFAICT there is no reason to have two different defines. I can submit
> > a small patch consolidating these, but it would remove the above hunk
> > anyway so I need to know whether you'll revert or not.
> 
> Thanks. I'll revert the patch so please prepare your patch without the
> check.

OK.

Thomas