diff mbox series

libertas: Fix two buffer overflows at parsing bss descriptor

Message ID 20191122052917.11309-1-huangwenabc@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series libertas: Fix two buffer overflows at parsing bss descriptor | expand

Commit Message

huangwenabc@gmail.com Nov. 22, 2019, 5:29 a.m. UTC 
From: Wen Huang <huangwenabc@gmail.com>

add_ie_rates() copys rates without checking the length 
in bss descriptor from remote AP.when victim connects to 
remote attacker, this may trigger buffer overflow.
lbs_ibss_join_existing() copys rates without checking the length 
in bss descriptor from remote IBSS node.when victim connects to 
remote attacker, this may trigger buffer overflow.
Fix them by putting the length check before performing copy.

This fix addresses CVE-2019-14896 and CVE-2019-14897.

Signed-off-by: Wen Huang <huangwenabc@gmail.com>
---
 drivers/net/wireless/marvell/libertas/cfg.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Kalle Valo Nov. 25, 2019, 12:36 p.m. UTC | #1 
kbuild test robot <lkp@intel.com> writes:

> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on wireless-drivers-next/master]
> [also build test WARNING on v5.4-rc8 next-20191122]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>
> url:    https://github.com/0day-ci/linux/commits/huangwenabc-gmail-com/libertas-Fix-two-buffer-overflows-at-parsing-bss-descriptor/20191124-142236
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
> config: sh-allmodconfig (attached as .config)
> compiler: sh4-linux-gcc (GCC) 7.4.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         GCC_VERSION=7.4.0 make.cross ARCH=sh 
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All warnings (new ones prefixed by >>):
>
>    drivers/net/wireless/marvell/libertas/cfg.c: In function 'lbs_ibss_join_existing':
>>> drivers/net/wireless/marvell/libertas/cfg.c:1788:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]

I was wondering why I didn't see this mail in patchwork:

https://patchwork.kernel.org/patch/11257187/

And then I noticed this:

X-Patchwork-Hint: ignore

kbuild team, why are you adding that header? It's really bad for a
maintainer like me who uses patchwork actively, it means that all these
important warnings are not visible in patchwork and can be easily missed
by the maintainers.
Philip Li Nov. 25, 2019, 2:29 p.m. UTC | #2 
On Mon, Nov 25, 2019 at 12:36:50PM +0000, Kalle Valo wrote:
> kbuild test robot <lkp@intel.com> writes:
> 
> > Thank you for the patch! Perhaps something to improve:
> >
> > [auto build test WARNING on wireless-drivers-next/master]
> > [also build test WARNING on v5.4-rc8 next-20191122]
> > [if your patch is applied to the wrong git tree, please drop us a note to help
> > improve the system. BTW, we also suggest to use '--base' option to specify the
> > base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> >
> > url:    https://github.com/0day-ci/linux/commits/huangwenabc-gmail-com/libertas-Fix-two-buffer-overflows-at-parsing-bss-descriptor/20191124-142236
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
> > config: sh-allmodconfig (attached as .config)
> > compiler: sh4-linux-gcc (GCC) 7.4.0
> > reproduce:
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # save the attached .config to linux build tree
> >         GCC_VERSION=7.4.0 make.cross ARCH=sh 
> >
> > If you fix the issue, kindly add following tag
> > Reported-by: kbuild test robot <lkp@intel.com>
> >
> > All warnings (new ones prefixed by >>):
> >
> >    drivers/net/wireless/marvell/libertas/cfg.c: In function 'lbs_ibss_join_existing':
> >>> drivers/net/wireless/marvell/libertas/cfg.c:1788:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
> 
> I was wondering why I didn't see this mail in patchwork:
> 
> https://patchwork.kernel.org/patch/11257187/
> 
> And then I noticed this:
> 
> X-Patchwork-Hint: ignore
> 
> kbuild team, why are you adding that header? It's really bad for a
thanks for the feedback, early on we received another feedback to suggest
for adding this, refer to https://gitlab.freedesktop.org/patchwork-fdo/patchwork-fdo/issues/21
for detail. Since there's no further input regarding this usage, we keep
that flag. If this is not suitable, we can investigate other way to fullfill
both requirements.

> maintainer like me who uses patchwork actively, it means that all these
> important warnings are not visible in patchwork and can be easily missed
> by the maintainers.
> 
> -- 
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
> _______________________________________________
> kbuild-all mailing list -- kbuild-all@lists.01.org
> To unsubscribe send an email to kbuild-all-leave@lists.01.org
Guenter Roeck Nov. 27, 2019, 6:23 p.m. UTC | #3 
On Mon, Nov 25, 2019 at 10:29:52PM +0800, Philip Li wrote:
> On Mon, Nov 25, 2019 at 12:36:50PM +0000, Kalle Valo wrote:
> > kbuild test robot <lkp@intel.com> writes:
> > 
> > > Thank you for the patch! Perhaps something to improve:
> > >
> > > [auto build test WARNING on wireless-drivers-next/master]
> > > [also build test WARNING on v5.4-rc8 next-20191122]
> > > [if your patch is applied to the wrong git tree, please drop us a note to help
> > > improve the system. BTW, we also suggest to use '--base' option to specify the
> > > base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> > >
> > > url:    https://github.com/0day-ci/linux/commits/huangwenabc-gmail-com/libertas-Fix-two-buffer-overflows-at-parsing-bss-descriptor/20191124-142236
> > > base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
> > > config: sh-allmodconfig (attached as .config)
> > > compiler: sh4-linux-gcc (GCC) 7.4.0
> > > reproduce:
> > >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > >         chmod +x ~/bin/make.cross
> > >         # save the attached .config to linux build tree
> > >         GCC_VERSION=7.4.0 make.cross ARCH=sh 
> > >
> > > If you fix the issue, kindly add following tag
> > > Reported-by: kbuild test robot <lkp@intel.com>
> > >
> > > All warnings (new ones prefixed by >>):
> > >
> > >    drivers/net/wireless/marvell/libertas/cfg.c: In function 'lbs_ibss_join_existing':
> > >>> drivers/net/wireless/marvell/libertas/cfg.c:1788:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
> > 
> > I was wondering why I didn't see this mail in patchwork:
> > 
> > https://patchwork.kernel.org/patch/11257187/
> > 
> > And then I noticed this:
> > 
> > X-Patchwork-Hint: ignore
> > 
> > kbuild team, why are you adding that header? It's really bad for a
> thanks for the feedback, early on we received another feedback to suggest
> for adding this, refer to https://gitlab.freedesktop.org/patchwork-fdo/patchwork-fdo/issues/21
> for detail. Since there's no further input regarding this usage, we keep
> that flag. If this is not suitable, we can investigate other way to fullfill
> both requirements.
> 

I second Kalle's comment; this is really bad.

Note that the above referenced link suggested to add
	X-Patchwork-Hint: comment
to e-mail headers. Instead, you added:
	X-Patchwork-Hint: ignore
which is substantially different. Also, the problem was with a _patch_
sent by the robot, not with direct feedback. On top of that, the
suggestion was really to add "X-Patchwork-Hint: comment" to _patches_
sent by the robot, not to everything. It should be fine to add
"X-Patchwork-Hint: ignore" to patches only as long as other feedback
is still provided and added to patchwork. That should meet all
requirements.

Thanks,
Guenter

> > maintainer like me who uses patchwork actively, it means that all these
> > important warnings are not visible in patchwork and can be easily missed
> > by the maintainers.
> > 
> > -- 
> > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
> > _______________________________________________
> > kbuild-all mailing list -- kbuild-all@lists.01.org
> > To unsubscribe send an email to kbuild-all-leave@lists.01.org
Chen, Rong A Nov. 28, 2019, 1:53 a.m. UTC | #4 
On 11/28/19 2:23 AM, Guenter Roeck wrote:
> On Mon, Nov 25, 2019 at 10:29:52PM +0800, Philip Li wrote:
>> On Mon, Nov 25, 2019 at 12:36:50PM +0000, Kalle Valo wrote:
>>> kbuild test robot <lkp@intel.com> writes:
>>>
>>>> Thank you for the patch! Perhaps something to improve:
>>>>
>>>> [auto build test WARNING on wireless-drivers-next/master]
>>>> [also build test WARNING on v5.4-rc8 next-20191122]
>>>> [if your patch is applied to the wrong git tree, please drop us a note to help
>>>> improve the system. BTW, we also suggest to use '--base' option to specify the
>>>> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>>>>
>>>> url:    https://github.com/0day-ci/linux/commits/huangwenabc-gmail-com/libertas-Fix-two-buffer-overflows-at-parsing-bss-descriptor/20191124-142236
>>>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
>>>> config: sh-allmodconfig (attached as .config)
>>>> compiler: sh4-linux-gcc (GCC) 7.4.0
>>>> reproduce:
>>>>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>>>>          chmod +x ~/bin/make.cross
>>>>          # save the attached .config to linux build tree
>>>>          GCC_VERSION=7.4.0 make.cross ARCH=sh
>>>>
>>>> If you fix the issue, kindly add following tag
>>>> Reported-by: kbuild test robot <lkp@intel.com>
>>>>
>>>> All warnings (new ones prefixed by >>):
>>>>
>>>>     drivers/net/wireless/marvell/libertas/cfg.c: In function 'lbs_ibss_join_existing':
>>>>>> drivers/net/wireless/marvell/libertas/cfg.c:1788:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
>>> I was wondering why I didn't see this mail in patchwork:
>>>
>>> https://patchwork.kernel.org/patch/11257187/
>>>
>>> And then I noticed this:
>>>
>>> X-Patchwork-Hint: ignore
>>>
>>> kbuild team, why are you adding that header? It's really bad for a
>> thanks for the feedback, early on we received another feedback to suggest
>> for adding this, refer to https://gitlab.freedesktop.org/patchwork-fdo/patchwork-fdo/issues/21
>> for detail. Since there's no further input regarding this usage, we keep
>> that flag. If this is not suitable, we can investigate other way to fullfill
>> both requirements.
>>
> I second Kalle's comment; this is really bad.
>
> Note that the above referenced link suggested to add
> 	X-Patchwork-Hint: comment
> to e-mail headers. Instead, you added:
> 	X-Patchwork-Hint: ignore
> which is substantially different. Also, the problem was with a _patch_
> sent by the robot, not with direct feedback. On top of that, the
> suggestion was really to add "X-Patchwork-Hint: comment" to _patches_
> sent by the robot, not to everything. It should be fine to add
> "X-Patchwork-Hint: ignore" to patches only as long as other feedback
> is still provided and added to patchwork. That should meet all
> requirements.
>
> Thanks,
> Guenter

Hi Kalle, Guenter

Thanks so much for your help, we have removed the hint header in build 
report mails and
still keep it in patch mails.

Best Regards,
Rong Chen

>
>>> maintainer like me who uses patchwork actively, it means that all these
>>> important warnings are not visible in patchwork and can be easily missed
>>> by the maintainers.
>>>
>>> -- 
>>> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
>>> _______________________________________________
>>> kbuild-all mailing list -- kbuild-all@lists.01.org
>>> To unsubscribe send an email to kbuild-all-leave@lists.01.org
> _______________________________________________
> kbuild-all mailing list -- kbuild-all@lists.01.org
> To unsubscribe send an email to kbuild-all-leave@lists.01.org
Kalle Valo Nov. 28, 2019, 8 a.m. UTC | #5 
huangwenabc@gmail.com wrote:

> From: Wen Huang <huangwenabc@gmail.com>
> 
> add_ie_rates() copys rates without checking the length 
> in bss descriptor from remote AP.when victim connects to 
> remote attacker, this may trigger buffer overflow.
> lbs_ibss_join_existing() copys rates without checking the length 
> in bss descriptor from remote IBSS node.when victim connects to 
> remote attacker, this may trigger buffer overflow.
> Fix them by putting the length check before performing copy.
> 
> This fix addresses CVE-2019-14896 and CVE-2019-14897.
> 
> Signed-off-by: Wen Huang <huangwenabc@gmail.com>

Please fix the warning reported by kbuild bot.

Patch set to Changes Requested.
Kalle Valo Nov. 28, 2019, 11:54 a.m. UTC | #6 
Wen Huang <huangwenabc@gmail.com> writes:

> I have modified the patch and submmit:
> https://patchwork.kernel.org/patch/11265751/ 

Thanks, but few tips for the future (no need to resend because of
these):

* don't use HTML in email

* use v2, v3 and so on to identify the version of the patch

* do not top post

More info in the link below, I suggest to carefully study that. Better
chances of getting your patches accepted that way.
Nicolai Stange Jan. 9, 2020, 2:12 p.m. UTC | #7 
Hi,

the patch queued as e5e884b42639 ("libertas: Fix two buffer overflows at
parsing bss descriptor") at the wireless tree doesn't look completely
correct to me.

This hunk here...

diff --git a/drivers/net/wireless/marvell/libertas/cfg.c b/drivers/net/wireless/marvell/libertas/cfg.c
index 57edfada0665..c9401c121a14 100644
--- a/drivers/net/wireless/marvell/libertas/cfg.c
+++ b/drivers/net/wireless/marvell/libertas/cfg.c
@@ -1775,9 +1782,12 @@ static int lbs_ibss_join_existing(struct lbs_private *priv,
 	if (!rates_eid) {
 		lbs_add_rates(cmd.bss.rates);
 	} else {
-		int hw, i;
-		u8 rates_max = rates_eid[1];
-		u8 *rates = cmd.bss.rates;
+		rates_max = rates_eid[1];
+		if (rates_max > MAX_RATES) {
+			lbs_deb_join("invalid rates");
+			goto out;

... makes the error path skip over the rcu_read_unlock() following later
in the code and leaves the RCU read section unbalanced.

Also, I'm wondering if ret should perhaps get set to some -Exxxx in case
of rates_max > MAX_RATES?

Thanks,

Nicolai


+		}
+		rates = cmd.bss.rates;
 		for (hw = 0; hw < ARRAY_SIZE(lbs_rates); hw++) {
 			u8 hw_rate = lbs_rates[hw].bitrate / 5;
 			for (i = 0; i < rates_max; i++) {
Kalle Valo March 24, 2020, 3:19 p.m. UTC | #8 
Rong Chen <rong.a.chen@intel.com> writes:

> On 11/28/19 2:23 AM, Guenter Roeck wrote:
>> On Mon, Nov 25, 2019 at 10:29:52PM +0800, Philip Li wrote:
>>> On Mon, Nov 25, 2019 at 12:36:50PM +0000, Kalle Valo wrote:
>>>> kbuild test robot <lkp@intel.com> writes:
>>>>
>>>>> Thank you for the patch! Perhaps something to improve:
>>>>>
>>>>> [auto build test WARNING on wireless-drivers-next/master]
>>>>> [also build test WARNING on v5.4-rc8 next-20191122]
>>>>> [if your patch is applied to the wrong git tree, please drop us a note to help
>>>>> improve the system. BTW, we also suggest to use '--base' option to specify the
>>>>> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>>>>>
>>>>> url:    https://github.com/0day-ci/linux/commits/huangwenabc-gmail-com/libertas-Fix-two-buffer-overflows-at-parsing-bss-descriptor/20191124-142236
>>>>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
>>>>> config: sh-allmodconfig (attached as .config)
>>>>> compiler: sh4-linux-gcc (GCC) 7.4.0
>>>>> reproduce:
>>>>>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>>>>>          chmod +x ~/bin/make.cross
>>>>>          # save the attached .config to linux build tree
>>>>>          GCC_VERSION=7.4.0 make.cross ARCH=sh
>>>>>
>>>>> If you fix the issue, kindly add following tag
>>>>> Reported-by: kbuild test robot <lkp@intel.com>
>>>>>
>>>>> All warnings (new ones prefixed by >>):
>>>>>
>>>>>     drivers/net/wireless/marvell/libertas/cfg.c: In function 'lbs_ibss_join_existing':
>>>>>>> drivers/net/wireless/marvell/libertas/cfg.c:1788:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
>>>> I was wondering why I didn't see this mail in patchwork:
>>>>
>>>> https://patchwork.kernel.org/patch/11257187/
>>>>
>>>> And then I noticed this:
>>>>
>>>> X-Patchwork-Hint: ignore
>>>>
>>>> kbuild team, why are you adding that header? It's really bad for a
>>> thanks for the feedback, early on we received another feedback to suggest
>>> for adding this, refer to
>>> https://gitlab.freedesktop.org/patchwork-fdo/patchwork-fdo/issues/21
>>> for detail. Since there's no further input regarding this usage, we keep
>>> that flag. If this is not suitable, we can investigate other way to fullfill
>>> both requirements.
>>>
>> I second Kalle's comment; this is really bad.
>>
>> Note that the above referenced link suggested to add
>> 	X-Patchwork-Hint: comment
>> to e-mail headers. Instead, you added:
>> 	X-Patchwork-Hint: ignore
>> which is substantially different. Also, the problem was with a _patch_
>> sent by the robot, not with direct feedback. On top of that, the
>> suggestion was really to add "X-Patchwork-Hint: comment" to _patches_
>> sent by the robot, not to everything. It should be fine to add
>> "X-Patchwork-Hint: ignore" to patches only as long as other feedback
>> is still provided and added to patchwork. That should meet all
>> requirements.
>>
>> Thanks,
>> Guenter
>
> Hi Kalle, Guenter
>
> Thanks so much for your help, we have removed the hint header in build
> report mails and still keep it in patch mails.

This is now working perfectly, here's a recent example:

https://patchwork.kernel.org/patch/11431301/

I cannot stress enough how much seeing kbuild bot warning in patchwork
helps my work as a maintainer. So thank you Guenter for the support and
Rong fixing it!
diff mbox series

Patch

diff --git a/drivers/net/wireless/marvell/libertas/cfg.c b/drivers/net/wireless/marvell/libertas/cfg.c
index 57edfada0..290280764 100644
--- a/drivers/net/wireless/marvell/libertas/cfg.c
+++ b/drivers/net/wireless/marvell/libertas/cfg.c
@@ -273,6 +273,10 @@  add_ie_rates(u8 *tlv, const u8 *ie, int *nrates)
 	int hw, ap, ap_max = ie[1];
 	u8 hw_rate;
 
+	if (ap_max > MAX_RATES) {
+		lbs_deb_assoc("invalid rates\n");
+		return tlv;
+	}
 	/* Advance past IE header */
 	ie += 2;
 
@@ -1777,6 +1781,10 @@  static int lbs_ibss_join_existing(struct lbs_private *priv,
 	} else {
 		int hw, i;
 		u8 rates_max = rates_eid[1];
+		if (rates_max > MAX_RATES) {
+			lbs_deb_join("invalid rates");
+			goto out;
+		}
 		u8 *rates = cmd.bss.rates;
 		for (hw = 0; hw < ARRAY_SIZE(lbs_rates); hw++) {
 			u8 hw_rate = lbs_rates[hw].bitrate / 5;