Re: [PATCH v4 01/10] wil6210: protect against invalid length of tx management frame

[Arend van Spriel]

On 8/3/2017 9:08 PM, Maya Erez wrote:

From: Hamad Kadmany 

Validate buffer length has the minimum needed size
when sending management frame to protect against
possible buffer overrun.


I noticed this is already applied, but I saw this subject text which has 
similar sound to a recent patch in our driver so I it made me curious...



Signed-off-by: Hamad Kadmany 
Signed-off-by: Lior David 
Signed-off-by: Maya Erez 
---
  drivers/net/wireless/ath/wil6210/cfg80211.c | 3 +++
  drivers/net/wireless/ath/wil6210/debugfs.c  | 3 +++
  2 files changed, 6 insertions(+)

diff --git a/drivers/net/wireless/ath/wil6210/cfg80211.c 
b/drivers/net/wireless/ath/wil6210/cfg80211.c
index 0b5383a..77af749 100644
--- a/drivers/net/wireless/ath/wil6210/cfg80211.c
+++ b/drivers/net/wireless/ath/wil6210/cfg80211.c
@@ -884,6 +884,9 @@ int wil_cfg80211_mgmt_tx(struct wiphy *wiphy, struct 
wireless_dev *wdev,
wil_hex_dump_misc("mgmt tx frame ", DUMP_PREFIX_OFFSET, 16, 1, buf,
  len, true);

+   if (len < sizeof(struct ieee80211_hdr_3addr))
+   return -EINVAL;


A similar check is already in net/wireless/cfg80211_mlme_mgmt_tx() which 
calls this function:


if (params->len < 24 + 1)
return -EINVAL;

So it only makes sense if this is called from some other call site


cmd = kmalloc(sizeof(*cmd) + len, GFP_KERNEL);
if (!cmd) {
rc = -ENOMEM;
diff --git a/drivers/net/wireless/ath/wil6210/debugfs.c 
b/drivers/net/wireless/ath/wil6210/debugfs.c
index f82506d..a2b5d59 100644
--- a/drivers/net/wireless/ath/wil6210/debugfs.c
+++ b/drivers/net/wireless/ath/wil6210/debugfs.c
@@ -801,6 +801,9 @@ static ssize_t wil_write_file_txmgmt(struct file *file, 
const char __user *buf,
int rc;
void *frame;

+   if (!len)
+   return -EINVAL;
+


... which is in this function. Now I wonder why you would need this 
method in the first place. Why not stick with using the nl80211 
NL80211_CMD_FRAME api?


Regards,
Arend


frame = memdup_user(buf, len);
if (IS_ERR(frame))
return PTR_ERR(frame);

[PATCH v4 01/10] wil6210: protect against invalid length of tx management frame

[Maya Erez]

From: Hamad Kadmany 

Validate buffer length has the minimum needed size
when sending management frame to protect against
possible buffer overrun.

Signed-off-by: Hamad Kadmany 
Signed-off-by: Lior David 
Signed-off-by: Maya Erez 
---
 drivers/net/wireless/ath/wil6210/cfg80211.c | 3 +++
 drivers/net/wireless/ath/wil6210/debugfs.c  | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/net/wireless/ath/wil6210/cfg80211.c 
b/drivers/net/wireless/ath/wil6210/cfg80211.c
index 0b5383a..77af749 100644
--- a/drivers/net/wireless/ath/wil6210/cfg80211.c
+++ b/drivers/net/wireless/ath/wil6210/cfg80211.c
@@ -884,6 +884,9 @@ int wil_cfg80211_mgmt_tx(struct wiphy *wiphy, struct 
wireless_dev *wdev,
wil_hex_dump_misc("mgmt tx frame ", DUMP_PREFIX_OFFSET, 16, 1, buf,
  len, true);
 
+   if (len < sizeof(struct ieee80211_hdr_3addr))
+   return -EINVAL;
+
cmd = kmalloc(sizeof(*cmd) + len, GFP_KERNEL);
if (!cmd) {
rc = -ENOMEM;
diff --git a/drivers/net/wireless/ath/wil6210/debugfs.c 
b/drivers/net/wireless/ath/wil6210/debugfs.c
index f82506d..a2b5d59 100644
--- a/drivers/net/wireless/ath/wil6210/debugfs.c
+++ b/drivers/net/wireless/ath/wil6210/debugfs.c
@@ -801,6 +801,9 @@ static ssize_t wil_write_file_txmgmt(struct file *file, 
const char __user *buf,
int rc;
void *frame;
 
+   if (!len)
+   return -EINVAL;
+
frame = memdup_user(buf, len);
if (IS_ERR(frame))
return PTR_ERR(frame);
-- 
1.9.1