Information leak in llcp_sock_bind/llcp_raw_sock_bind

[Dmitry Vyukov]

Hello,

The following program leads to leak of unint bytes from kernel stack:

#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 

#define NFC_SOCKPROTO_LLCP 1

int main(void)
{
struct sockaddr sa;
unsigned len, i, try;
int fd;

for (try = 0; try < 3; try++) {
fd = socket(AF_NFC, 3, NFC_SOCKPROTO_LLCP);
if (fd == -1)
return;
switch (try) {
case 0:
break;
case 1:
sched_yield();
break;
case 2:
open("/dev/null", O_RDONLY);
}
memset(, 0, sizeof(sa));
sa.sa_family = AF_NFC;
bind(fd, , 2);
len = sizeof(sa);
getsockname(fd, , );
for (i = 0; i < len; i++)
printf("%02x", ((unsigned char*))[i]);
printf("\n");
}
return 0;
}

Output:
2700b002401f4511e5e38f90b002400018006c007c13410028f77610fe7f5e104000
2700b002400212046c164769b002400018006c007c134100c874fff4fe7f5e104000
2700b002408e8a91e4e069fcb002400018006c007c134100f868b3f2fe7f5e104000

The problem is that llcp_sock_bind/llcp_raw_sock_bind do not check
sockaddr_len passed in, so they copy stack garbage from stack into the
socket and then return it in getsockname.
This can defeat ASLR, leak crypto keys, etc.
--
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

Re: [PATCH] nl80211: Fix potential memory leak in nl80211_connect

[Johannes Berg]

On Fri, 2015-12-11 at 21:04 +0100, Ola Olsson wrote:
> Free cached keys if the last early return path is taken.
> 
Also applied, thanks.

johannes
--
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

Re: [PATCH] nl80211: Potential memory leaks in reg.c

[Johannes Berg]

On Sun, 2015-12-13 at 19:12 +0100, Ola Olsson wrote:
> The first leak occurs when entering the default case
> in the switch for the initiator in set_regdom.
> The second leaks a platform_device struct if the
> platform registration in regulatory_init succeeds but
> the sub sequent regulatory hint fails due to no memory.

Applied, thanks. How are you finding these?? They seems so obscure :)

johannes
--
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

Re: [PATCH] nl80211: Potential memory leaks in reg.c

[Ola Olsson]

I'm trying to learn how the code works so I'm reading through it line
by line. I have seen so many memory leaks in my days so I know where
to look ;)
They are indeed obscure since we need an error to actually trigger
them but I thought I might as well share the info when I found them.

On Tue, Dec 15, 2015 at 1:09 PM, Johannes Berg
 wrote:
> On Sun, 2015-12-13 at 19:12 +0100, Ola Olsson wrote:
>> The first leak occurs when entering the default case
>> in the switch for the initiator in set_regdom.
>> The second leaks a platform_device struct if the
>> platform registration in regulatory_init succeeds but
>> the sub sequent regulatory hint fails due to no memory.
>
> Applied, thanks. How are you finding these?? They seems so obscure :)
>
> johannes
--
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

Re: Information leak in llcp_sock_bind/llcp_raw_sock_bind

[Dmitry Vyukov]

On Tue, Dec 15, 2015 at 9:36 PM, David Miller  wrote:
> From: Dmitry Vyukov 
> Date: Tue, 15 Dec 2015 21:00:20 +0100
>
>> The problem is that llcp_sock_bind/llcp_raw_sock_bind do not check
>> sockaddr_len passed in, so they copy stack garbage from stack into the
>> socket and then return it in getsockname.
>> This can defeat ASLR, leak crypto keys, etc.
>
> That's actually the first thing these functions do.
>
> They completely clear out the on-stack llcp_addr, then they copy only
> as much as the user gave them, being careful not to use more than
> sizeof(llcp_addr).
>
> memset(_addr, 0, sizeof(llcp_addr));
> len = min_t(unsigned int, sizeof(llcp_addr), alen);
> memcpy(_addr, addr, len);
>
> I don't see what the problem is, you'll need to be more specific.

You are right. Sorry.

There still seems to be a minor leak here:

  if (!addr || addr->sa_family != AF_NFC)
  return -EINVAL;

addr->sa_family can be uninit.
--
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

Re: Information leak in llcp_sock_bind/llcp_raw_sock_bind

[David Miller]

From: Dmitry Vyukov 
Date: Tue, 15 Dec 2015 21:45:16 +0100

> On Tue, Dec 15, 2015 at 9:36 PM, David Miller  wrote:
>> From: Dmitry Vyukov 
>> Date: Tue, 15 Dec 2015 21:00:20 +0100
>>
>>> The problem is that llcp_sock_bind/llcp_raw_sock_bind do not check
>>> sockaddr_len passed in, so they copy stack garbage from stack into the
>>> socket and then return it in getsockname.
>>> This can defeat ASLR, leak crypto keys, etc.
>>
>> That's actually the first thing these functions do.
>>
>> They completely clear out the on-stack llcp_addr, then they copy only
>> as much as the user gave them, being careful not to use more than
>> sizeof(llcp_addr).
>>
>> memset(_addr, 0, sizeof(llcp_addr));
>> len = min_t(unsigned int, sizeof(llcp_addr), alen);
>> memcpy(_addr, addr, len);
>>
>> I don't see what the problem is, you'll need to be more specific.
> 
> You are right. Sorry.
> 
> There still seems to be a minor leak here:
> 
>   if (!addr || addr->sa_family != AF_NFC)
>   return -EINVAL;
> 
> addr->sa_family can be uninit.

That shouldn't matter at all, that can't cause socket state corruption.

I want to ask you if you are actually seeing kernel stack in that hexdump
you are posting?  If so, how do you actually account for it?  Nothing you
have shown so far make that clear.
--
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

[PATCH] staging: rtl8723au: change parameter type in rtl8723a_set_rssi_cmd declaration

[drivengroove]

From: Anatoly Stepanov 

Previosly the function had the following prototype:
int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u8 *param)

My suggestion here is to modify the prototype this way:
int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param)

We can do this based on the following considerations:
1. rtl8723a_set_rssi_cmd is used only with 32-bit "param" values
2. There's no point in using "__u8 *param" until we pass param length

Signed-off-by: Anatoly Stepanov 
---
 drivers/staging/rtl8723au/hal/odm.c  | 2 +-
 drivers/staging/rtl8723au/hal/rtl8723a_cmd.c | 6 +++---
 drivers/staging/rtl8723au/include/rtl8723a_cmd.h | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/rtl8723au/hal/odm.c 
b/drivers/staging/rtl8723au/hal/odm.c
index 6b9dbef..6fed13e 100644
--- a/drivers/staging/rtl8723au/hal/odm.c
+++ b/drivers/staging/rtl8723au/hal/odm.c
@@ -1274,7 +1274,7 @@ static void odm_RSSIMonitorCheck(struct dm_odm_t *pDM_Odm)
 
for (i = 0; i < sta_cnt; i++) {
if (PWDB_rssi[i] != (0))
-   rtl8723a_set_rssi_cmd(Adapter, (u8 *)_rssi[i]);
+   rtl8723a_set_rssi_cmd(Adapter, _rssi[i]);
}
 
pdmpriv->EntryMaxUndecoratedSmoothedPWDB = MaxDB;
diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c 
b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
index 1662c03c..e899d05 100644
--- a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
+++ b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
@@ -113,11 +113,11 @@ exit:
return ret;
 }
 
-int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u8 *param)
+int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param)
 {
-   *((u32 *)param) = cpu_to_le32(*((u32 *)param));
+   __le32 cmd = cpu_to_le32(*param);
 
-   FillH2CCmd(padapter, RSSI_SETTING_EID, 3, param);
+   FillH2CCmd(padapter, RSSI_SETTING_EID, 3, (void *));
 
return _SUCCESS;
 }
diff --git a/drivers/staging/rtl8723au/include/rtl8723a_cmd.h 
b/drivers/staging/rtl8723au/include/rtl8723a_cmd.h
index 014c02e..e39e38a 100644
--- a/drivers/staging/rtl8723au/include/rtl8723a_cmd.h
+++ b/drivers/staging/rtl8723au/include/rtl8723a_cmd.h
@@ -149,7 +149,7 @@ void rtl8723a_set_BTCoex_AP_mode_FwRsvdPkt_cmd(struct 
rtw_adapter *padapter);
 #else
 #define rtl8723a_set_BTCoex_AP_mode_FwRsvdPkt_cmd(padapter) do {} while(0)
 #endif
-int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u8 *param);
+int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param);
 int rtl8723a_set_raid_cmd(struct rtw_adapter *padapter, u32 mask, u8 arg);
 void rtl8723a_add_rateatid(struct rtw_adapter *padapter, u32 bitmap, u8 arg, 
u8 rssi_level);
 
-- 
1.9.1

--
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

Re: Information leak in llcp_sock_bind/llcp_raw_sock_bind

[Dmitry Vyukov]

On Tue, Dec 15, 2015 at 9:48 PM, David Miller  wrote:
> From: Dmitry Vyukov 
> Date: Tue, 15 Dec 2015 21:45:16 +0100
>
>> On Tue, Dec 15, 2015 at 9:36 PM, David Miller  wrote:
>>> From: Dmitry Vyukov 
>>> Date: Tue, 15 Dec 2015 21:00:20 +0100
>>>
 The problem is that llcp_sock_bind/llcp_raw_sock_bind do not check
 sockaddr_len passed in, so they copy stack garbage from stack into the
 socket and then return it in getsockname.
 This can defeat ASLR, leak crypto keys, etc.
>>>
>>> That's actually the first thing these functions do.
>>>
>>> They completely clear out the on-stack llcp_addr, then they copy only
>>> as much as the user gave them, being careful not to use more than
>>> sizeof(llcp_addr).
>>>
>>> memset(_addr, 0, sizeof(llcp_addr));
>>> len = min_t(unsigned int, sizeof(llcp_addr), alen);
>>> memcpy(_addr, addr, len);
>>>
>>> I don't see what the problem is, you'll need to be more specific.
>>
>> You are right. Sorry.
>>
>> There still seems to be a minor leak here:
>>
>>   if (!addr || addr->sa_family != AF_NFC)
>>   return -EINVAL;
>>
>> addr->sa_family can be uninit.
>
> That shouldn't matter at all, that can't cause socket state corruption.
>
> I want to ask you if you are actually seeing kernel stack in that hexdump
> you are posting?  If so, how do you actually account for it?  Nothing you
> have shown so far make that clear.

I've seen a kernel address at least in pptp_bind, it was a return pc
in SyS_socket call that was executed just before bind.
Exact contents of the leaked info depend on kernel config, compiler
and a previous executed syscall (there are thousands of them if we
count ioctls and friends). So it is almost impossible to prove that a
PC cannot be leaked.
--
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

Re: Information leak in llcp_sock_bind/llcp_raw_sock_bind

[David Miller]

From: Dmitry Vyukov 
Date: Tue, 15 Dec 2015 21:00:20 +0100

> The problem is that llcp_sock_bind/llcp_raw_sock_bind do not check
> sockaddr_len passed in, so they copy stack garbage from stack into the
> socket and then return it in getsockname.
> This can defeat ASLR, leak crypto keys, etc.

That's actually the first thing these functions do.

They completely clear out the on-stack llcp_addr, then they copy only
as much as the user gave them, being careful not to use more than
sizeof(llcp_addr).

memset(_addr, 0, sizeof(llcp_addr));
len = min_t(unsigned int, sizeof(llcp_addr), alen);
memcpy(_addr, addr, len);

I don't see what the problem is, you'll need to be more specific.

Even wrt. llcp_sock->service_name, the code limits the string to
NFC_LLCP_MAX_SERVICE_NAME.
--
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

Re: [PATCH] staging: rtl8723au: use proper typecast in assignment

[Jes Sorensen]

Anatoly Stepanov  writes:
> This resolves the following Sparse warning:
> "incorrect type in assignment (different base types)"
>
> Signed-off-by: Anatoly Stepanov 
> ---
>  drivers/staging/rtl8723au/hal/rtl8723a_cmd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c 
> b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> index 1662c03c..fdd9cf1 100644
> --- a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> +++ b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> @@ -115,7 +115,7 @@ exit:
>  
>  int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u8 *param)
>  {
> - *((u32 *)param) = cpu_to_le32(*((u32 *)param));
> + *((__le32 *)param) = cpu_to_le32(*((u32 *)param));
>  
>   FillH2CCmd(padapter, RSSI_SETTING_EID, 3, param);

*Cough*

Writing a 32 bit value to a pointer to a u8 is ugly in the first
place. Rather than just wrapping it in yet another ugly typecast, please
fix this properly.

Jes
--
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

question on "mac80211_hwsim: support any address in userspace"

[Ben Greear]

This patch below was added to the kernel around 2/24/2015

I am curious mostly about the first change:  I thought the transmitter-addr
relates to the radio device, not the vdev (sta, ap, etc).

But, wouldn't using data from the header break that assumption?


Is there any actual advantage to having more than one address per
hwsim radio?  It seems it complicates things for no particular
reason as far as I can tell?

Thanks,
Ben


mac80211_hwsim: support any address in userspace

Due to the checks in get_hwsim_data_ref_from_addr, wmediumd
was only able to use the second mac address (those starting with
0x42).  This is confusing and needlessly limiting, so allow any
configured address.

Signed-off-by: Bob Copeland 
Signed-off-by: Johannes Berg 

 drivers/net/wireless/mac80211_hwsim.c 
index 4a4c658..e259ee1 100644
@@ -906,8 +906,7 @@ static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw 
*hw,
goto nla_put_failure;
}

-   if (nla_put(skb, HWSIM_ATTR_ADDR_TRANSMITTER,
-   ETH_ALEN, data->addresses[1].addr))
+   if (nla_put(skb, HWSIM_ATTR_ADDR_TRANSMITTER, ETH_ALEN, hdr->addr2))
goto nla_put_failure;

/* We get the skb->data */
@@ -2608,7 +2607,7 @@ static struct mac80211_hwsim_data 
*get_hwsim_data_ref_from_addr(const u8 *addr)

spin_lock_bh(_radio_lock);
list_for_each_entry(data, _radios, list) {
-   if (memcmp(data->addresses[1].addr, addr, ETH_ALEN) == 0) {
+   if (mac80211_hwsim_addr_match(data, addr)) {
_found = true;
break;
}


--
Ben Greear 
Candela Technologies Inc  http://www.candelatech.com

--
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

pull-request: mac80211 2015-12-15

[Johannes Berg]

Hi Dave,

After going through my patch queue, I have another set of fixes that I
think is still appropriate for the current cycle.

Two of my own restart changes there are fairly big but for the most
part just move code around so it can be called in a slightly different
order.

Let me know if there are any issues.

Thanks,
johannes


The following changes since commit c1df932c0574c13ab3ce72e969c9647ff3aaad68:

  mac80211: fix off-channel mgmt-tx uninitialized variable usage (2015-12-02 
22:27:53 +0100)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git 
tags/mac80211-for-davem-2015-12-15

for you to fetch changes up to cf1e05c63642ce65821a6277adfc2157f7334c9d:

  mac80211: handle width changes from opmode notification IE in beacon 
(2015-12-15 13:16:47 +0100)


Another set of fixes:
 * memory leak fixes (from Ola)
 * operating mode notification spec compliance fix (from Eyal)
 * copy rfkill names in case pointer becomes invalid (myself)
 * two hardware restart fixes (myself)
 * get rid of "limiting TX power" log spam (myself)


Eyal Shapira (1):
  mac80211: handle width changes from opmode notification IE in beacon

Johannes Berg (4):
  rfkill: copy the name into the rfkill struct
  mac80211: run scan completed work on reconfig failure
  mac80211: reprogram in interface order
  mac80211: suppress unchanged "limiting TX power" messages

Ola Olsson (3):
  nl80211: fix a few memory leaks in reg.c
  nl80211: Fix potential memory leak in nl80211_set_wowlan
  nl80211: Fix potential memory leak in nl80211_connect

 net/mac80211/cfg.c |   3 +-
 net/mac80211/ieee80211_i.h |   4 +-
 net/mac80211/mlme.c|  17 ---
 net/mac80211/rx.c  |   3 +-
 net/mac80211/util.c| 113 +
 net/mac80211/vht.c |  10 ++--
 net/rfkill/core.c  |   6 +--
 net/wireless/nl80211.c |   5 +-
 net/wireless/reg.c |   5 +-
 9 files changed, 92 insertions(+), 74 deletions(-)
--
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

Re: [PATCH] nl80211: Potential memory leaks in reg.c

[Johannes Berg]

On Tue, 2015-12-15 at 13:19 +0100, Ola Olsson wrote:
> I'm trying to learn how the code works so I'm reading through it line
> by line. I have seen so many memory leaks in my days so I know where
> to look ;)

Ok :)

> They are indeed obscure since we need an error to actually trigger
> them but I thought I might as well share the info when I found them.

Of course, the patches are very welcome!

I'm just surprised that none of the tools we (and others) typically run
found these, and thought perhaps you had a better tool than we do...
I guess it's called "brain" and we just don't use it right ;-)

johannes
--
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

Re: Information leak in llcp_sock_bind/llcp_raw_sock_bind

[David Miller]

From: Dmitry Vyukov 
Date: Tue, 15 Dec 2015 21:55:37 +0100

> I've seen a kernel address at least in pptp_bind,

We're not talking about pptp_bind.

We're talking about llcp_{,raw}_sock_bind().

If your hex dump doesn't show it, don't report anything unless you are
absolutely sure via code inspection that there could be a leak.  And
in that case make it perfectly clear exactly how that can happen.

I am generally unimpressed with your reports half of the time,
and just a small amount of extra effort would extraordinarily
improve the quality of the things your post.

Thanks.

> So it is almost impossible to prove that a PC cannot be leaked.

You can't show that anything is actually being leaked in this specific
case, period.
--
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

Re: iwlwifi - L1 Enabled - LTR Enabled loop

[Emmanuel Grumbach]

On Mon, Dec 14, 2015 at 10:48 AM, Johannes Berg
 wrote:
> On Mon, 2015-12-14 at 04:00 +, Grumbach, Emmanuel wrote:
>>
>> This is really weird. I don't see how that could be firmware related.
>> BTW, -10.ucode is fairly old :) we have -16.ucode out there.
>
> I guess the question is how we get into this situation to start with?
>
> About the only thing I can imagine is that we fail somehow during
> initialization (without printing any other message) and higher layers
> like wpa_supplicant/network-manager/... simply try to bring up the
> device over and over again?
>

I can reproduce this now when I unload / load iwlwifi until I kill the
supplicant.
Seems like the wpa_s is indeed toggling the device.
I don't have time to play with that right now.
--
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