2013-02-07 13:44:50

by Jiri Slaby

[permalink] [raw]
Subject: [PATCH] NET: ath5k, check ath5k_eeprom_mode_from_channel retval

It can, if invalid argument given, return a negative value. In that
case we would access arrays out-of-bounds and such. Check the value
and yell loudly if that happened as it would be a bug in the
implementation. (Instead of silently corrupting memory.)

Signed-off-by: Jiri Slaby <[email protected]>
Cc: Nick Kossifidis <[email protected]>
Cc: "Luis R. Rodriguez" <[email protected]>
---
drivers/net/wireless/ath/ath5k/phy.c | 4 ++++
drivers/net/wireless/ath/ath5k/reset.c | 2 ++
2 files changed, 6 insertions(+)

diff --git a/drivers/net/wireless/ath/ath5k/phy.c b/drivers/net/wireless/ath/ath5k/phy.c
index ab363f3..a78afa9 100644
--- a/drivers/net/wireless/ath/ath5k/phy.c
+++ b/drivers/net/wireless/ath/ath5k/phy.c
@@ -1613,6 +1613,10 @@ ath5k_hw_update_noise_floor(struct ath5k_hw *ah)
ah->ah_cal_mask |= AR5K_CALIBRATION_NF;

ee_mode = ath5k_eeprom_mode_from_channel(ah->ah_current_channel);
+ if (WARN_ON(ee_mode < 0)) {
+ ah->ah_cal_mask &= ~AR5K_CALIBRATION_NF;
+ return;
+ }

/* completed NF calibration, test threshold */
nf = ath5k_hw_read_measured_noise_floor(ah);
diff --git a/drivers/net/wireless/ath/ath5k/reset.c b/drivers/net/wireless/ath/ath5k/reset.c
index 4084b10..e2d8b2c 100644
--- a/drivers/net/wireless/ath/ath5k/reset.c
+++ b/drivers/net/wireless/ath/ath5k/reset.c
@@ -985,6 +985,8 @@ ath5k_hw_commit_eeprom_settings(struct ath5k_hw *ah,
return;

ee_mode = ath5k_eeprom_mode_from_channel(channel);
+ if (WARN_ON(ee_mode < 0))
+ return;

/* Adjust power delta for channel 14 */
if (channel->center_freq == 2484)
--
1.8.1.1




2013-02-19 13:36:17

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH] NET: ath5k, check ath5k_eeprom_mode_from_channel retval

On 02/18/2013 01:47 AM, Nick Kossifidis wrote:
> int
> ath5k_eeprom_mode_from_channel(struct ieee80211_channel *channel)
> {
> switch (channel->hw_value) {
> case AR5K_MODE_11A:
> return AR5K_EEPROM_MODE_11A;
> case AR5K_MODE_11G:
> return AR5K_EEPROM_MODE_11G;
> case AR5K_MODE_11B:
> return AR5K_EEPROM_MODE_11B;
> default:
> return -1;
> }
> }
>
> I think we should just change that default to return 0 instead and add
> an ATH5K_WARN there.

Something like the attached patch? It needs ah to be propagated to
eeprom. If you are fine with that, I'll send it as patch...

thanks,
--
js
suse labs


Attachments:
0001-ath5k-cleanup-channel-to-eprom_mode-function.patch (3.95 kB)

2013-02-19 14:54:57

by Nick Kossifidis

[permalink] [raw]
Subject: Re: [PATCH] NET: ath5k, check ath5k_eeprom_mode_from_channel retval

On Tue Feb 19 15:36:07 2013, Jiri Slaby wrote:
> On 02/18/2013 01:47 AM, Nick Kossifidis wrote:
>> int
>> ath5k_eeprom_mode_from_channel(struct ieee80211_channel *channel)
>> {
>> switch (channel->hw_value) {
>> case AR5K_MODE_11A:
>> return AR5K_EEPROM_MODE_11A;
>> case AR5K_MODE_11G:
>> return AR5K_EEPROM_MODE_11G;
>> case AR5K_MODE_11B:
>> return AR5K_EEPROM_MODE_11B;
>> default:
>> return -1;
>> }
>> }
>>
>> I think we should just change that default to return 0 instead and add
>> an ATH5K_WARN there.
>
> Something like the attached patch? It needs ah to be propagated to
> eeprom. If you are fine with that, I'll send it as patch...
>
> thanks,

Just move the prototype on ath5k.h with the rest of them...

1523 /* EEPROM access functions */
1524 int ath5k_eeprom_init(struct ath5k_hw *ah);
1525 void ath5k_eeprom_detach(struct ath5k_hw *ah);

2013-02-18 00:47:45

by Nick Kossifidis

[permalink] [raw]
Subject: Re: [PATCH] NET: ath5k, check ath5k_eeprom_mode_from_channel retval

2013/2/7 Jiri Slaby <[email protected]>:
> It can, if invalid argument given, return a negative value. In that
> case we would access arrays out-of-bounds and such. Check the value
> and yell loudly if that happened as it would be a bug in the
> implementation. (Instead of silently corrupting memory.)
>
> Signed-off-by: Jiri Slaby <[email protected]>
> Cc: Nick Kossifidis <[email protected]>
> Cc: "Luis R. Rodriguez" <[email protected]>
> ---
> drivers/net/wireless/ath/ath5k/phy.c | 4 ++++
> drivers/net/wireless/ath/ath5k/reset.c | 2 ++
> 2 files changed, 6 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath5k/phy.c b/drivers/net/wireless/ath/ath5k/phy.c
> index ab363f3..a78afa9 100644
> --- a/drivers/net/wireless/ath/ath5k/phy.c
> +++ b/drivers/net/wireless/ath/ath5k/phy.c
> @@ -1613,6 +1613,10 @@ ath5k_hw_update_noise_floor(struct ath5k_hw *ah)
> ah->ah_cal_mask |= AR5K_CALIBRATION_NF;
>
> ee_mode = ath5k_eeprom_mode_from_channel(ah->ah_current_channel);
> + if (WARN_ON(ee_mode < 0)) {
> + ah->ah_cal_mask &= ~AR5K_CALIBRATION_NF;
> + return;
> + }
>
> /* completed NF calibration, test threshold */
> nf = ath5k_hw_read_measured_noise_floor(ah);
> diff --git a/drivers/net/wireless/ath/ath5k/reset.c b/drivers/net/wireless/ath/ath5k/reset.c
> index 4084b10..e2d8b2c 100644
> --- a/drivers/net/wireless/ath/ath5k/reset.c
> +++ b/drivers/net/wireless/ath/ath5k/reset.c
> @@ -985,6 +985,8 @@ ath5k_hw_commit_eeprom_settings(struct ath5k_hw *ah,
> return;
>
> ee_mode = ath5k_eeprom_mode_from_channel(channel);
> + if (WARN_ON(ee_mode < 0))
> + return;
>
> /* Adjust power delta for channel 14 */
> if (channel->center_freq == 2484)

int
ath5k_eeprom_mode_from_channel(struct ieee80211_channel *channel)
{
switch (channel->hw_value) {
case AR5K_MODE_11A:
return AR5K_EEPROM_MODE_11A;
case AR5K_MODE_11G:
return AR5K_EEPROM_MODE_11G;
case AR5K_MODE_11B:
return AR5K_EEPROM_MODE_11B;
default:
return -1;
}
}

I think we should just change that default to return 0 instead and add
an ATH5K_WARN there.



--
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick