[PATCH] net: 6pack: fix slab-out-of-bounds in decode_data

Syzbot reported slab-out-of bounds write in decode_data().
The problem was in missing validation checks.

Syzbot's reproducer generated malicious input, which caused
decode_data() to be called a lot in sixpack_decode(). Since
rx_count_cooked is only 400 bytes and noone reported before,
that 400 bytes is not enough, let's just check if input is malicious
and complain about buffer overrun.

Fail log:
==================================================================
BUG: KASAN: slab-out-of-bounds in drivers/net/hamradio/6pack.c:843
Write of size 1 at addr ffff888087c5544e by task kworker/u4:0/7

CPU: 0 PID: 7 Comm: kworker/u4:0 Not tainted 5.6.0-rc3-syzkaller #0
...
Workqueue: events_unbound flush_to_ldisc
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x197/0x210 lib/dump_stack.c:118
 print_address_description.constprop.0.cold+0xd4/0x30b mm/kasan/report.c:374
 __kasan_report.cold+0x1b/0x32 mm/kasan/report.c:506
 kasan_report+0x12/0x20 mm/kasan/common.c:641
 __asan_report_store1_noabort+0x17/0x20 mm/kasan/generic_report.c:137
 decode_data.part.0+0x23b/0x270 drivers/net/hamradio/6pack.c:843
 decode_data drivers/net/hamradio/6pack.c:965 [inline]
 sixpack_decode drivers/net/hamradio/6pack.c:968 [inline]

Reported-and-tested-by: syzbot+fc8cd9a673d4577fb2e4@syzkaller.appspotmail.com
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Pavel Skripkin <redacted>
---
 drivers/net/hamradio/6pack.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
index fcf3af76b6d7..f4ffc2a80ab7 100644
--- a/drivers/net/hamradio/6pack.c
+++ b/drivers/net/hamradio/6pack.c

@@ -827,6 +827,12 @@ static void decode_data(struct sixpack *sp, unsigned char inbyte)
 		return;
 	}
 
+	if (sp->rx_count_cooked + 3 >= sizeof(sp->cooked_buf)) {
+		pr_err("6pack: cooked buffer overrun, data loss\n");
+		sp->rx_count = 0;
+		return;
+	}
+
 	buf = sp->raw_buf;
 	sp->cooked_buf[sp->rx_count_cooked++] =
 		buf[0] | ((buf[1] << 2) & 0xc0);

-- 
2.32.0

On Fri, Aug 13, 2021 at 02:28:55PM +0300, Pavel Skripkin wrote:

It should be + 2 instead of + 3.

We write three bytes.  idx, idx + 1, idx + 2.  Otherwise, good fix!

regards,
dan carpenter

On 8/13/21 5:58 PM, Dan Carpenter wrote:

On Fri, Aug 13, 2021 at 02:28:55PM +0300, Pavel Skripkin wrote:

quoted

Syzbot reported slab-out-of bounds write in decode_data().
The problem was in missing validation checks.

Syzbot's reproducer generated malicious input, which caused
decode_data() to be called a lot in sixpack_decode(). Since
rx_count_cooked is only 400 bytes and noone reported before,
that 400 bytes is not enough, let's just check if input is malicious
and complain about buffer overrun.

Fail log:
==================================================================
BUG: KASAN: slab-out-of-bounds in drivers/net/hamradio/6pack.c:843
Write of size 1 at addr ffff888087c5544e by task kworker/u4:0/7

CPU: 0 PID: 7 Comm: kworker/u4:0 Not tainted 5.6.0-rc3-syzkaller #0
...
Workqueue: events_unbound flush_to_ldisc
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x197/0x210 lib/dump_stack.c:118
 print_address_description.constprop.0.cold+0xd4/0x30b mm/kasan/report.c:374
 __kasan_report.cold+0x1b/0x32 mm/kasan/report.c:506
 kasan_report+0x12/0x20 mm/kasan/common.c:641
 __asan_report_store1_noabort+0x17/0x20 mm/kasan/generic_report.c:137
 decode_data.part.0+0x23b/0x270 drivers/net/hamradio/6pack.c:843
 decode_data drivers/net/hamradio/6pack.c:965 [inline]
 sixpack_decode drivers/net/hamradio/6pack.c:968 [inline]

Reported-and-tested-by: syzbot+fc8cd9a673d4577fb2e4@syzkaller.appspotmail.com
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Pavel Skripkin <redacted>
---
 drivers/net/hamradio/6pack.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
index fcf3af76b6d7..f4ffc2a80ab7 100644
--- a/drivers/net/hamradio/6pack.c
+++ b/drivers/net/hamradio/6pack.c

@@ -827,6 +827,12 @@ static void decode_data(struct sixpack *sp, unsigned char inbyte)
 		return;
 	}
 
+	if (sp->rx_count_cooked + 3 >= sizeof(sp->cooked_buf)) {

It should be + 2 instead of + 3.

We write three bytes.  idx, idx + 1, idx + 2.  Otherwise, good fix!

Indeed. Will fix in v2, thank you for pointing it out!


With regards,
Pavel Skripkin

Syzbot reported slab-out-of bounds write in decode_data().
The problem was in missing validation checks.

Syzbot's reproducer generated malicious input, which caused
decode_data() to be called a lot in sixpack_decode(). Since
rx_count_cooked is only 400 bytes and noone reported before,
that 400 bytes is not enough, let's just check if input is malicious
and complain about buffer overrun.

Fail log:
==================================================================
BUG: KASAN: slab-out-of-bounds in drivers/net/hamradio/6pack.c:843
Write of size 1 at addr ffff888087c5544e by task kworker/u4:0/7

CPU: 0 PID: 7 Comm: kworker/u4:0 Not tainted 5.6.0-rc3-syzkaller #0
...
Workqueue: events_unbound flush_to_ldisc
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x197/0x210 lib/dump_stack.c:118
 print_address_description.constprop.0.cold+0xd4/0x30b mm/kasan/report.c:374
 __kasan_report.cold+0x1b/0x32 mm/kasan/report.c:506
 kasan_report+0x12/0x20 mm/kasan/common.c:641
 __asan_report_store1_noabort+0x17/0x20 mm/kasan/generic_report.c:137
 decode_data.part.0+0x23b/0x270 drivers/net/hamradio/6pack.c:843
 decode_data drivers/net/hamradio/6pack.c:965 [inline]
 sixpack_decode drivers/net/hamradio/6pack.c:968 [inline]

Reported-and-tested-by: syzbot+fc8cd9a673d4577fb2e4@syzkaller.appspotmail.com
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Pavel Skripkin <redacted>
---

Changes in v2:
	+ 3 -> +2 (Reported by Dan Carpenter)

---
 drivers/net/hamradio/6pack.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
index fcf3af76b6d7..8fe8887d506a 100644
--- a/drivers/net/hamradio/6pack.c
+++ b/drivers/net/hamradio/6pack.c

@@ -827,6 +827,12 @@ static void decode_data(struct sixpack *sp, unsigned char inbyte)
 		return;
 	}
 
+	if (sp->rx_count_cooked + 2 >= sizeof(sp->cooked_buf)) {
+		pr_err("6pack: cooked buffer overrun, data loss\n");
+		sp->rx_count = 0;
+		return;
+	}
+
 	buf = sp->raw_buf;
 	sp->cooked_buf[sp->rx_count_cooked++] =
 		buf[0] | ((buf[1] << 2) & 0xc0);

-- 
2.32.0

Great!

Reviewed-by: Dan Carpenter <redacted>

regards,
dan carpenter

On 8/14/21 12:09 AM, Dan Carpenter wrote:

Great!

Reviewed-by: Dan Carpenter <redacted>

Thank you, Dan!


With regards,
Pavel Skripkin

On Fri, Aug 13, 2021 at 05:58:34PM +0300, Dan Carpenter wrote:

On Fri, Aug 13, 2021 at 02:28:55PM +0300, Pavel Skripkin wrote:

quoted

Syzbot reported slab-out-of bounds write in decode_data().
The problem was in missing validation checks.

Syzbot's reproducer generated malicious input, which caused
decode_data() to be called a lot in sixpack_decode(). Since
rx_count_cooked is only 400 bytes and noone reported before,
that 400 bytes is not enough, let's just check if input is malicious
and complain about buffer overrun.

...

diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
index fcf3af76b6d7..f4ffc2a80ab7 100644
--- a/drivers/net/hamradio/6pack.c
+++ b/drivers/net/hamradio/6pack.c

@@ -827,6 +827,12 @@ static void decode_data(struct sixpack *sp, unsigned char inbyte)
 		return;
 	}
 
+	if (sp->rx_count_cooked + 3 >= sizeof(sp->cooked_buf)) {

It should be + 2 instead of + 3.

We write three bytes.  idx, idx + 1, idx + 2.  Otherwise, good fix!

I would suggest that the statement be:

	if (sp->rx_count_cooked + 3 > sizeof(sp->cooked_buf)) {

or even, because it's a buffer overrun test:

	if (sp->rx_count_cooked > sizeof(sp->cooked_buf) - 3) {

This is because if there are three bytes being written, that is the number that should be obvious in the test.

I haven't looked at the surrounding code and there may be some other consideration why the "+ 2 >=" rather than "+ 3 >" (and from the description of "idx, idx + 1, idx + 2", I suspect it's visual consistency), so if that is important, feel free to adjust as required.

Thanks,
Kevin

On 8/14/21 3:23 AM, Kevin Dawson wrote:

On Fri, Aug 13, 2021 at 05:58:34PM +0300, Dan Carpenter wrote:

quoted

On Fri, Aug 13, 2021 at 02:28:55PM +0300, Pavel Skripkin wrote:

quoted

Syzbot reported slab-out-of bounds write in decode_data().
The problem was in missing validation checks.

Syzbot's reproducer generated malicious input, which caused
decode_data() to be called a lot in sixpack_decode(). Since
rx_count_cooked is only 400 bytes and noone reported before,
that 400 bytes is not enough, let's just check if input is malicious
and complain about buffer overrun.

...

diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
index fcf3af76b6d7..f4ffc2a80ab7 100644
--- a/drivers/net/hamradio/6pack.c
+++ b/drivers/net/hamradio/6pack.c

@@ -827,6 +827,12 @@ static void decode_data(struct sixpack *sp, unsigned char inbyte)
 		return;
 	}
 
+	if (sp->rx_count_cooked + 3 >= sizeof(sp->cooked_buf)) {

It should be + 2 instead of + 3.

We write three bytes.  idx, idx + 1, idx + 2.  Otherwise, good fix!

I would suggest that the statement be:

	if (sp->rx_count_cooked + 3 > sizeof(sp->cooked_buf)) {

or even, because it's a buffer overrun test:

	if (sp->rx_count_cooked > sizeof(sp->cooked_buf) - 3) {

Hmm, I think, it will be more straightforward for someone not aware 
about driver details.

@Dan, can I add your Reviewed-by tag to v3 and what do you think about 
Kevin's suggestion?

This is because if there are three bytes being written, that is the number that should be obvious in the test.

I haven't looked at the surrounding code and there may be some other consideration why the "+ 2 >=" rather than "+ 3 >" (and from the description of "idx, idx + 1, idx + 2", I suspect it's visual consistency), so if that is important, feel free to adjust as required.

With regards,
Pavel Skripkin

On Sat, Aug 14, 2021 at 05:17:44PM +0300, Pavel Skripkin wrote:

On 8/14/21 3:23 AM, Kevin Dawson wrote:

quoted

On Fri, Aug 13, 2021 at 05:58:34PM +0300, Dan Carpenter wrote:

quoted

On Fri, Aug 13, 2021 at 02:28:55PM +0300, Pavel Skripkin wrote:

quoted

Syzbot reported slab-out-of bounds write in decode_data().
The problem was in missing validation checks.

quoted

Syzbot's reproducer generated malicious input, which caused

decode_data() to be called a lot in sixpack_decode(). Since
rx_count_cooked is only 400 bytes and noone reported before,
that 400 bytes is not enough, let's just check if input is malicious
and complain about buffer overrun.

quoted

...

diff --git a/drivers/net/hamradio/6pack.c

b/drivers/net/hamradio/6pack.c

quoted

index fcf3af76b6d7..f4ffc2a80ab7 100644

--- a/drivers/net/hamradio/6pack.c
+++ b/drivers/net/hamradio/6pack.c

@@ -827,6 +827,12 @@ static void decode_data(struct sixpack *sp, unsigned char inbyte)
 		return;
 	}
 > +	if (sp->rx_count_cooked + 3 >= sizeof(sp->cooked_buf)) {

It should be + 2 instead of + 3.

We write three bytes.  idx, idx + 1, idx + 2.  Otherwise, good fix!

I would suggest that the statement be:

	if (sp->rx_count_cooked + 3 > sizeof(sp->cooked_buf)) {

or even, because it's a buffer overrun test:

	if (sp->rx_count_cooked > sizeof(sp->cooked_buf) - 3) {

Hmm, I think, it will be more straightforward for someone not aware about
driver details.

@Dan, can I add your Reviewed-by tag to v3 and what do you think about
Kevin's suggestion?

I don't care.  Sure.  I'm also fine with leaving it as is.  I've been
using "idx + 2 >= sizeof()" enough recently that it has become an idiom
for me.  But that's probably a bias on my part.

I guess "idx + 3 > sizeof()" is probably the most readable.  Moving
the + 3 to the other side would prevent integer overflows but we're not
concerned about that here and no need to over engineer things if it
hurts readability.

regards,
dan carpenter

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Fri, 13 Aug 2021 18:14:33 +0300 you wrote:

Syzbot reported slab-out-of bounds write in decode_data().
The problem was in missing validation checks.

Syzbot's reproducer generated malicious input, which caused
decode_data() to be called a lot in sixpack_decode(). Since
rx_count_cooked is only 400 bytes and noone reported before,
that 400 bytes is not enough, let's just check if input is malicious
and complain about buffer overrun.

[...]

Here is the summary with links:
  - [v2] net: 6pack: fix slab-out-of-bounds in decode_data
    https://git.kernel.org/netdev/net/c/19d1532a1876

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html