| Message ID | 1501787302-22885-2-git-send-email-qca_merez@qca.qualcomm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6641525ce40ef45641c8f43bb19cd4e471e4cb75 |
| Delegated to: | Kalle Valo |
| Headers | show |
Maya Erez <qca_merez@qca.qualcomm.com> wrote: > Validate buffer length has the minimum needed size > when sending management frame to protect against > possible buffer overrun. > > Signed-off-by: Hamad Kadmany <qca_hkadmany@qca.qualcomm.com> > Signed-off-by: Lior David <qca_liord@qca.qualcomm.com> > Signed-off-by: Maya Erez <qca_merez@qca.qualcomm.com> > Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com> 8 patches applied to ath-next branch of ath.git, thanks. 6641525ce40e wil6210: protect against invalid length of tx management frame 30868f5d4413 wil6210: support FW RSSI reporting c6622116c5ae wil6210: check no_fw_recovery in resume failure recovery 262345265e59 wil6210: add statistics for suspend time d1fbf07540b7 wil6210: notify wiphy on wowlan support 9b2a4c2d534c wil6210: fix interface-up check eb4c02155881 wil6210: store FW RF calibration result 38d16ab2b213 wil6210: move vring_idle_trsh definition to wil6210_priv
On 8/3/2017 9:08 PM, Maya Erez wrote: > From: Hamad Kadmany <qca_hkadmany@qca.qualcomm.com> > > 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 <qca_hkadmany@qca.qualcomm.com> > Signed-off-by: Lior David <qca_liord@qca.qualcomm.com> > Signed-off-by: Maya Erez <qca_merez@qca.qualcomm.com> > --- > 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); >
On 8/9/2017 11:02 AM, Arend van Spriel wrote: > On 8/3/2017 9:08 PM, Maya Erez wrote: >> From: Hamad Kadmany <qca_hkadmany@qca.qualcomm.com> >> >> 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 <qca_hkadmany@qca.qualcomm.com> >> Signed-off-by: Lior David <qca_liord@qca.qualcomm.com> >> Signed-off-by: Maya Erez <qca_merez@qca.qualcomm.com> >> --- >> 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? > Using NL80211_CMD_FRAME reaches cfg80211_mlme_mgmt_tx which performs validity checks such as: - Fail to send frame types not supported by the driver - Some strict checks for non-public action frames. The debugfs file provides a convenient and simple way to send any type of management frames for debugging and testing. For example, we use it with some testing scripts, basically turning a station into a "packet injector" and sending frames into the air for simulating various scenarios. Thanks, Lior
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);