Re: [PATCH 2/2] mwifiex: fix unaligned read in mwifiex_config_scan()

2016-09-08 Thread Kalle Valo 
Amitkumar Karwar  writes:

>> > --- a/drivers/net/wireless/marvell/mwifiex/scan.c
>> > +++ b/drivers/net/wireless/marvell/mwifiex/scan.c
>> > @@ -883,7 +883,8 @@ mwifiex_config_scan(struct mwifiex_private *priv,
>> >sizeof(scan_cfg_out->specific_bssid));
>> >
>> > if (adapter->ext_scan &&
>> > -   !is_zero_ether_addr(scan_cfg_out->specific_bssid))
>> {
>> > +   !is_zero_ether_addr_unaligned(
>> > +   scan_cfg_out->specific_bssid)) {
>> 
>> Any comments? Is this approach of adding
>> is_zero_ether_addr_unaligned() fine? We already have similar routine
>> ether_addr_equal_unaligned().
>> 
>> I don't see much benefit making a local, aligned copy here. It would
>> have to use memcpy w/ byte operations anyways and then still run
>> is_zero_ether_addr().
>> 
>> Amitkumar -- Is it possible to modify struct mwifiex_scan_cmd_config {}
>> and align specific_bssid field to u16 boundary?
>> 
>
> We can’t change the structure. The reason is firmware at receiving end
> expects the variables in the same order.
> is_zero_ether_addr_unaligned() should be fine.

If I understood correctly Dave doesn't like
is_zero_ether_addr_unaligned() so we can't use that either.

-- 
Kalle Valo

RE: [PATCH 2/2] mwifiex: fix unaligned read in mwifiex_config_scan()

2016-09-08 Thread Amitkumar Karwar 
Hi Petri,

> -Original Message-
> From: Petri Gynther [mailto:pgynt...@google.com]
> Sent: Tuesday, August 16, 2016 11:17 PM
> To: linux-wireless@vger.kernel.org
> Cc: kv...@codeaurora.org; David Miller; Joe Perches; Amitkumar Karwar;
> Petri Gynther
> Subject: Re: [PATCH 2/2] mwifiex: fix unaligned read in
> mwifiex_config_scan()
> 
> On Fri, Aug 12, 2016 at 8:00 PM, Petri Gynther 
> wrote:
> > $ iwconfig mlan0 essid MySSID
> > [   36.93] Path: /sbin/iwconfig
> > [   36.93] CPU: 0 PID: 203 Comm: iwconfig Not tainted 4.7.0 #2
> > [   36.94] task: 866f83a0 ti: 866a6000 task.ti: 866a6000
> > [   36.94]
> > [ECR   ]: 0x00230400 => Misaligned r/w from 0x8677f403
> > [   36.96] [EFA   ]: 0x8677f403
> > [   36.96] [BLINK ]: mwifiex_scan_networks+0x17a/0x198c [mwifiex]
> > [   36.96] [ERET  ]: mwifiex_scan_networks+0x18a/0x198c [mwifiex]
> > [   36.98] [STAT32]: 0x0206 : K E2 E1
> > [   36.98] BTA: 0x700736e2   SP: 0x866a7d0c  FP: 0x5faddc84
> > [   37.00] LPS: 0x806a37ec  LPE: 0x806a37fa LPC: 0x
> > [   37.00] r00: 0x8677f401  r01: 0x8668aa08 r02: 0x0001
> > r03: 0x r04: 0x8668b600 r05: 0x8677f406
> > r06: 0x8702b600 r07: 0x r08: 0x8702b600
> > r09: 0x r10: 0x870b3b00 r11: 0x
> > r12: 0x
> > [   37.04]
> > [   37.04] Stack Trace:
> > [   37.04]   mwifiex_scan_networks+0x18a/0x198c [mwifiex]
> >
> > Root cause:
> > mwifiex driver calls is_zero_ether_addr() against byte-aligned
> address:
> >
> > drivers/net/wireless/marvell/mwifiex/fw.h:
> > struct mwifiex_scan_cmd_config {
> > /*
> >  *  BSS mode to be sent in the firmware command
> >  */
> > u8 bss_mode;
> >
> > /* Specific BSSID used to filter scan results in the firmware
> */
> > u8 specific_bssid[ETH_ALEN];
> >
> > ...
> > } __packed;
> >
> > drivers/net/wireless/marvell/mwifiex/scan.c:
> > mwifiex_config_scan(..., struct mwifiex_scan_cmd_config *scan_cfg_out,
> ...)
> > ...
> > if (adapter->ext_scan &&
> > !is_zero_ether_addr(scan_cfg_out->specific_bssid)) {
> > ...
> > }
> >
> > Since firmware-related struct mwifiex_scan_cmd_config cannot be
> > changed, we need to use the new function
> is_zero_ether_addr_unaligned() here.
> >
> > This is v2 of the original patch:
> > [PATCH] Modify is_zero_ether_addr() to handle byte-aligned addresses
> >
> > Per Joe's suggestion -- instead of modifying is_zero_ether_addr() --
> > add is_zero_ether_addr_unaligned() and use it where needed.
> >
> > Cc: Kalle Valo 
> > Cc: David S. Miller 
> > Cc: Joe Perches 
> > Cc: Amitkumar Karwar 
> > Signed-off-by: Petri Gynther 
> > ---
> >  drivers/net/wireless/marvell/mwifiex/scan.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c
> > b/drivers/net/wireless/marvell/mwifiex/scan.c
> > index bc5e52c..d648c88 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/scan.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/scan.c
> > @@ -883,7 +883,8 @@ mwifiex_config_scan(struct mwifiex_private *priv,
> >sizeof(scan_cfg_out->specific_bssid));
> >
> > if (adapter->ext_scan &&
> > -   !is_zero_ether_addr(scan_cfg_out->specific_bssid))
> {
> > +   !is_zero_ether_addr_unaligned(
> > +   scan_cfg_out->specific_bssid)) {
> 
> Any comments? Is this approach of adding
> is_zero_ether_addr_unaligned() fine? We already have similar routine
> ether_addr_equal_unaligned().
> 
> I don't see much benefit making a local, aligned copy here. It would
> have to use memcpy w/ byte operations anyways and then still run
> is_zero_ether_addr().
> 
> Amitkumar -- Is it possible to modify struct mwifiex_scan_cmd_config {}
> and align specific_bssid field to u16 boundary?
> 

We can’t change the structure. The reason is firmware at receiving end expects 
the variables in the same order.
is_zero_ether_addr_unaligned() should be fine.

Regards,
Amitkumar

Re: [PATCH 2/2] mwifiex: fix unaligned read in mwifiex_config_scan()

2016-08-16 Thread Petri Gynther 
On Fri, Aug 12, 2016 at 8:00 PM, Petri Gynther  wrote:
> $ iwconfig mlan0 essid MySSID
> [   36.93] Path: /sbin/iwconfig
> [   36.93] CPU: 0 PID: 203 Comm: iwconfig Not tainted 4.7.0 #2
> [   36.94] task: 866f83a0 ti: 866a6000 task.ti: 866a6000
> [   36.94]
> [ECR   ]: 0x00230400 => Misaligned r/w from 0x8677f403
> [   36.96] [EFA   ]: 0x8677f403
> [   36.96] [BLINK ]: mwifiex_scan_networks+0x17a/0x198c [mwifiex]
> [   36.96] [ERET  ]: mwifiex_scan_networks+0x18a/0x198c [mwifiex]
> [   36.98] [STAT32]: 0x0206 : K E2 E1
> [   36.98] BTA: 0x700736e2   SP: 0x866a7d0c  FP: 0x5faddc84
> [   37.00] LPS: 0x806a37ec  LPE: 0x806a37fa LPC: 0x
> [   37.00] r00: 0x8677f401  r01: 0x8668aa08 r02: 0x0001
> r03: 0x r04: 0x8668b600 r05: 0x8677f406
> r06: 0x8702b600 r07: 0x r08: 0x8702b600
> r09: 0x r10: 0x870b3b00 r11: 0x
> r12: 0x
> [   37.04]
> [   37.04] Stack Trace:
> [   37.04]   mwifiex_scan_networks+0x18a/0x198c [mwifiex]
>
> Root cause:
> mwifiex driver calls is_zero_ether_addr() against byte-aligned address:
>
> drivers/net/wireless/marvell/mwifiex/fw.h:
> struct mwifiex_scan_cmd_config {
> /*
>  *  BSS mode to be sent in the firmware command
>  */
> u8 bss_mode;
>
> /* Specific BSSID used to filter scan results in the firmware */
> u8 specific_bssid[ETH_ALEN];
>
> ...
> } __packed;
>
> drivers/net/wireless/marvell/mwifiex/scan.c:
> mwifiex_config_scan(..., struct mwifiex_scan_cmd_config *scan_cfg_out, ...)
> ...
> if (adapter->ext_scan &&
> !is_zero_ether_addr(scan_cfg_out->specific_bssid)) {
> ...
> }
>
> Since firmware-related struct mwifiex_scan_cmd_config cannot be changed,
> we need to use the new function is_zero_ether_addr_unaligned() here.
>
> This is v2 of the original patch:
> [PATCH] Modify is_zero_ether_addr() to handle byte-aligned addresses
>
> Per Joe's suggestion -- instead of modifying is_zero_ether_addr() --
> add is_zero_ether_addr_unaligned() and use it where needed.
>
> Cc: Kalle Valo 
> Cc: David S. Miller 
> Cc: Joe Perches 
> Cc: Amitkumar Karwar 
> Signed-off-by: Petri Gynther 
> ---
>  drivers/net/wireless/marvell/mwifiex/scan.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c 
> b/drivers/net/wireless/marvell/mwifiex/scan.c
> index bc5e52c..d648c88 100644
> --- a/drivers/net/wireless/marvell/mwifiex/scan.c
> +++ b/drivers/net/wireless/marvell/mwifiex/scan.c
> @@ -883,7 +883,8 @@ mwifiex_config_scan(struct mwifiex_private *priv,
>sizeof(scan_cfg_out->specific_bssid));
>
> if (adapter->ext_scan &&
> -   !is_zero_ether_addr(scan_cfg_out->specific_bssid)) {
> +   !is_zero_ether_addr_unaligned(
> +   scan_cfg_out->specific_bssid)) {

Any comments? Is this approach of adding
is_zero_ether_addr_unaligned() fine? We already have similar routine
ether_addr_equal_unaligned().

I don't see much benefit making a local, aligned copy here. It would
have to use memcpy w/ byte operations anyways and then still run
is_zero_ether_addr().

Amitkumar -- Is it possible to modify struct mwifiex_scan_cmd_config
{} and align specific_bssid field to u16 boundary?

> bssid_tlv =
> (struct mwifiex_ie_types_bssid_list *)tlv_pos;
> bssid_tlv->header.type = cpu_to_le16(TLV_TYPE_BSSID);
> --
> 2.8.0.rc3.226.g39d4020
>

[PATCH 2/2] mwifiex: fix unaligned read in mwifiex_config_scan()

2016-08-12 Thread Petri Gynther 
$ iwconfig mlan0 essid MySSID
[   36.93] Path: /sbin/iwconfig
[   36.93] CPU: 0 PID: 203 Comm: iwconfig Not tainted 4.7.0 #2
[   36.94] task: 866f83a0 ti: 866a6000 task.ti: 866a6000
[   36.94]
[ECR   ]: 0x00230400 => Misaligned r/w from 0x8677f403
[   36.96] [EFA   ]: 0x8677f403
[   36.96] [BLINK ]: mwifiex_scan_networks+0x17a/0x198c [mwifiex]
[   36.96] [ERET  ]: mwifiex_scan_networks+0x18a/0x198c [mwifiex]
[   36.98] [STAT32]: 0x0206 : K E2 E1
[   36.98] BTA: 0x700736e2   SP: 0x866a7d0c  FP: 0x5faddc84
[   37.00] LPS: 0x806a37ec  LPE: 0x806a37fa LPC: 0x
[   37.00] r00: 0x8677f401  r01: 0x8668aa08 r02: 0x0001
r03: 0x r04: 0x8668b600 r05: 0x8677f406
r06: 0x8702b600 r07: 0x r08: 0x8702b600
r09: 0x r10: 0x870b3b00 r11: 0x
r12: 0x
[   37.04]
[   37.04] Stack Trace:
[   37.04]   mwifiex_scan_networks+0x18a/0x198c [mwifiex]

Root cause:
mwifiex driver calls is_zero_ether_addr() against byte-aligned address:

drivers/net/wireless/marvell/mwifiex/fw.h:
struct mwifiex_scan_cmd_config {
/*
 *  BSS mode to be sent in the firmware command
 */
u8 bss_mode;

/* Specific BSSID used to filter scan results in the firmware */
u8 specific_bssid[ETH_ALEN];

...
} __packed;

drivers/net/wireless/marvell/mwifiex/scan.c:
mwifiex_config_scan(..., struct mwifiex_scan_cmd_config *scan_cfg_out, ...)
...
if (adapter->ext_scan &&
!is_zero_ether_addr(scan_cfg_out->specific_bssid)) {
...
}

Since firmware-related struct mwifiex_scan_cmd_config cannot be changed,
we need to use the new function is_zero_ether_addr_unaligned() here.

This is v2 of the original patch:
[PATCH] Modify is_zero_ether_addr() to handle byte-aligned addresses

Per Joe's suggestion -- instead of modifying is_zero_ether_addr() --
add is_zero_ether_addr_unaligned() and use it where needed.

Cc: Kalle Valo 
Cc: David S. Miller 
Cc: Joe Perches 
Cc: Amitkumar Karwar 
Signed-off-by: Petri Gynther 
---
 drivers/net/wireless/marvell/mwifiex/scan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c 
b/drivers/net/wireless/marvell/mwifiex/scan.c
index bc5e52c..d648c88 100644
--- a/drivers/net/wireless/marvell/mwifiex/scan.c
+++ b/drivers/net/wireless/marvell/mwifiex/scan.c
@@ -883,7 +883,8 @@ mwifiex_config_scan(struct mwifiex_private *priv,
   sizeof(scan_cfg_out->specific_bssid));
 
if (adapter->ext_scan &&
-   !is_zero_ether_addr(scan_cfg_out->specific_bssid)) {
+   !is_zero_ether_addr_unaligned(
+   scan_cfg_out->specific_bssid)) {
bssid_tlv =
(struct mwifiex_ie_types_bssid_list *)tlv_pos;
bssid_tlv->header.type = cpu_to_le16(TLV_TYPE_BSSID);
-- 
2.8.0.rc3.226.g39d4020

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html