Re: [PATCH V2 1/3] brcmfmac: Avoid possible out-of-bounds read

[Kalle Valo]

Arend van Spriel  writes:

> On 09-09-17 21:30, Kevin Cernekee wrote:
>> In brcmf_p2p_notify_rx_mgmt_p2p_probereq(), chanspec is assigned before
>> the length of rxframe is validated.  This could lead to uninitialized
>> data being accessed (but not printed).  Since we already have a
>> perfectly good endian-swapped copy of rxframe->chanspec in ch.chspec,
>> and ch.chspec is not modified by decchspec(), avoid the extra
>> assignment and use ch.chspec in the debug print.
>>
>> Suggested-by: Mattias Nissler 
>> Signed-off-by: Kevin Cernekee 
>> Reviewed-by: Arend van Spriel 
>> ---
>>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>>
>> V1->V2: Clarify changelog re: whether the uninitialized data is printed.
>
> This patch and the others in this series look fine to me.

Should these go to v4.14?

-- 
Kalle Valo

RE: [PATCH] mwifiex: remove unnecessary call to memset

[Xinming Hu]

Hi Himanshu,


> -Original Message-
> From: Himanshu Jha [mailto:himanshujha199...@gmail.com]
> Sent: 2017年9月11日 20:46
> To: amitkar...@gmail.com
> Cc: Nishant Sarmukadam ; Ganapathi Bhat
> ; Xinming Hu ;
> kv...@codeaurora.org; linux-wireless@vger.kernel.org;
> net...@vger.kernel.org; Himanshu Jha 
> Subject: [EXT] [PATCH] mwifiex: remove unnecessary call to memset
> 
> External Email
> 
> --
> call to memset to assign 0 value immediately after allocating memory with
> kzalloc is unnecesaary as kzalloc allocates the memory filled with 0 value.
> 

Looks find to me, Thanks.

Regards,
Simon

> Semantic patch used to resolve this issue:
> 
> @@
> expression e,e2; constant c;
> statement S;
> @@
> 
>   e = kzalloc(e2, c);
>   if(e == NULL) S
> - memset(e, 0, e2);
> 
> Signed-off-by: Himanshu Jha 
> ---
>  drivers/net/wireless/marvell/mwifiex/scan.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c
> b/drivers/net/wireless/marvell/mwifiex/scan.c
> index c9d41ed..8838b88 100644
> --- a/drivers/net/wireless/marvell/mwifiex/scan.c
> +++ b/drivers/net/wireless/marvell/mwifiex/scan.c
> @@ -1936,8 +1936,6 @@ mwifiex_active_scan_req_for_passive_chan(struct
> mwifiex_private *priv)
>   if (!user_scan_cfg)
>   return -ENOMEM;
> 
> - memset(user_scan_cfg, 0, sizeof(*user_scan_cfg));
> -
>   for (id = 0; id < MWIFIEX_USER_SCAN_CHAN_MAX; id++) {
>   if (!priv->hidden_chan[id].chan_number)
>   break;
> --
> 2.7.4

Re: [PATCH 3/3] brcmfmac: Add check for short event packets

[Arend van Spriel]

On 11-09-17 11:19, Mattias Nissler wrote:

On Fri, Sep 8, 2017 at 9:13 PM, Kevin Cernekee  wrote:


The length of the data in the received skb is currently passed into
brcmf_fweh_process_event() as packet_len, but this value is not checked.
event_packet should be followed by DATALEN bytes of additional event
data.  Ensure that the received packet actually contains at least
DATALEN bytes of additional data, to avoid copying uninitialized memory
into event->data.

Suggested-by: Mattias Nissler 
Signed-off-by: Kevin Cernekee 
---
  drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
index 5aabdc9ed7e0..4cad1f0d2a82 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
@@ -429,7 +429,8 @@ void brcmf_fweh_process_event(struct brcmf_pub *drvr,
 if (code != BRCMF_E_IF && !fweh->evt_handler[code])
 return;

-   if (datalen > BRCMF_DCMD_MAXLEN)
+   if (datalen > BRCMF_DCMD_MAXLEN ||
+   datalen + sizeof(*event_packet) < packet_len)


Shouldn't this check be larger-than, i.e. we need the packet to be at
least sizeof(*event_packet) + its payload size?


That depends on how you formulate the requirement. packet_len here is 
the length for the received skbuff. The event message (= 
sizeof(*event_packet)) and its variable payload (= datalen) shall not 
exceed length of received skbuff (= packet_len).


Regards,
Arend

QCA6174 fw logs howto

[Bartosz Chronowski]

Hi everyone,
What are the steps necessary to read QCA6174 firmware logs?

I'm using:
echo 0x 0 > /sys/kernel/debug/ieee80211/phy*/ath10k/fw_dbglog
trace-cmd record -e ath10k
trace-cmd report > file

But cannot see fw logs. Are those in binary format in "file" ? If so ,
how to convert them  to readible format?

Regards,
BC

[PATCH] mwifiex: remove unnecessary call to memset

[Himanshu Jha]

call to memset to assign 0 value immediately after allocating
memory with kzalloc is unnecesaary as kzalloc allocates the memory
filled with 0 value.

Semantic patch used to resolve this issue:

@@
expression e,e2; constant c;
statement S;
@@

  e = kzalloc(e2, c);
  if(e == NULL) S
- memset(e, 0, e2);

Signed-off-by: Himanshu Jha 
---
 drivers/net/wireless/marvell/mwifiex/scan.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c 
b/drivers/net/wireless/marvell/mwifiex/scan.c
index c9d41ed..8838b88 100644
--- a/drivers/net/wireless/marvell/mwifiex/scan.c
+++ b/drivers/net/wireless/marvell/mwifiex/scan.c
@@ -1936,8 +1936,6 @@ mwifiex_active_scan_req_for_passive_chan(struct 
mwifiex_private *priv)
if (!user_scan_cfg)
return -ENOMEM;
 
-   memset(user_scan_cfg, 0, sizeof(*user_scan_cfg));
-
for (id = 0; id < MWIFIEX_USER_SCAN_CHAN_MAX; id++) {
if (!priv->hidden_chan[id].chan_number)
break;
-- 
2.7.4

Re: [RFC 2/4] mac80211_hwsim: add hwsim_tx_rate_flags to Netlink Attributes

[Benjamin Beichler]

Am 08.09.2017 um 16:26 schrieb Johannes Berg:
> On Fri, 2017-09-08 at 16:11 +0200, Benjamin Beichler wrote:
>> For correct interpretation of a tx rate, the corresponding rate flags
>> are needed (e.g. whether a HT-MCS rate or a legacy rate) and
>> moreover for more correct simulation the other infos of the flags are
>> important (like short-GI). Keeping compability, the flags
>> are not integrated into the existing hwsim_tx_rate, but transmitted
>> as an additional netlink attribute.
> This still exposes a lot of internal detail - perhaps it'd be better to
> convert to some sort of stable rate representation, like in cfg80211?
> Not sure that's sufficient though, it's a bit less detailed.
I don't know what is the problem with the details. The only flag, which
is a bit to verbose is  MAC80211_HWSIM_TX_RC_DUP_DATA, which we may
omit. All others describe directly terms used in the IEEE 802.11
standard. Also the representation, that a rate is an MCS-index is quite
good. If you take look here http://mcsindex.com/ , the bitrate would be
not sufficient to get the exact coding and fec rate, therefore you would
also need additional flags. You are right regarding legacy rates, which
are in an encoded table. I tried to decouple internal and external API,
but currently there is no big difference.

Nonetheless the whole hwsim API is highly specialized and only usable
with the linux kernel. Of course the Userland API should be more or less
stable, but the backward compatibility is not touched by this change. As
I already said, this is nearly a fix for hwsim, since currently it's
impossible to differentiate between legacy and MCS-rates, although they
could appear in a single tx_rates array. I think currently minstrel does
not mix HT and legacy rates for data frames, but AFAIK Management/Action
frames are always sent with legacy rates, so there are mixed already.


> johannes

kind regards

Benjamin

-- 
M.Sc. Benjamin Beichler

Universität Rostock, Fakultät für Informatik und Elektrotechnik
Institut für Angewandte Mikroelektronik und Datentechnik

University of Rostock, Department of CS and EE
Institute of Applied Microelectronics and CE

Richard-Wagner-Straße 31
18119 Rostock
Deutschland/Germany

phone: +49 (0) 381 498 - 7278
email: benjamin.beich...@uni-rostock.de
www: http://www.imd.uni-rostock.de/




smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH 3/3] brcmfmac: Add check for short event packets

[Mattias Nissler]

On Fri, Sep 8, 2017 at 9:13 PM, Kevin Cernekee  wrote:
>
> The length of the data in the received skb is currently passed into
> brcmf_fweh_process_event() as packet_len, but this value is not checked.
> event_packet should be followed by DATALEN bytes of additional event
> data.  Ensure that the received packet actually contains at least
> DATALEN bytes of additional data, to avoid copying uninitialized memory
> into event->data.
>
> Suggested-by: Mattias Nissler 
> Signed-off-by: Kevin Cernekee 
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
> index 5aabdc9ed7e0..4cad1f0d2a82 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fweh.c
> @@ -429,7 +429,8 @@ void brcmf_fweh_process_event(struct brcmf_pub *drvr,
> if (code != BRCMF_E_IF && !fweh->evt_handler[code])
> return;
>
> -   if (datalen > BRCMF_DCMD_MAXLEN)
> +   if (datalen > BRCMF_DCMD_MAXLEN ||
> +   datalen + sizeof(*event_packet) < packet_len)

Shouldn't this check be larger-than, i.e. we need the packet to be at
least sizeof(*event_packet) + its payload size?

> return;
>
> if (in_interrupt())
> --
> 2.14.1.581.gf28d330327-goog
>