sctp: fix uninit-value in sctp_inq_pop()

[PATCH net] sctp: fix uninit-value in sctp_inq_pop()

Posted by Nikita Zhandarovich 2 years, 3 months ago

Syzbot identified a case [1] of uninitialized memory usage in
sctp_inq_pop(), specifically in 'ch->length'.

Fix the issue by ensuring that 'ch->length' reflects the size of
'sctp_chunkhdr *ch' before accessing it.

[1]
BUG: KMSAN: uninit-value in sctp_inq_pop+0x1597/0x1910 net/sctp/inqueue.c:205
 sctp_inq_pop+0x1597/0x1910 net/sctp/inqueue.c:205
 sctp_assoc_bh_rcv+0x1a7/0xc50 net/sctp/associola.c:997
 sctp_inq_push+0x23e/0x2b0 net/sctp/inqueue.c:80
 sctp_backlog_rcv+0x394/0xd80 net/sctp/input.c:331
 sk_backlog_rcv include/net/sock.h:1115 [inline]
 __release_sock+0x207/0x570 net/core/sock.c:2911
 release_sock+0x6b/0x1e0 net/core/sock.c:3478
 sctp_wait_for_connect+0x486/0x810 net/sctp/socket.c:9325
 sctp_sendmsg_to_asoc+0x1ea7/0x1ee0 net/sctp/socket.c:1884
 ...

Uninit was stored to memory at:
 sctp_inq_pop+0x151a/0x1910 net/sctp/inqueue.c:201
 sctp_assoc_bh_rcv+0x1a7/0xc50 net/sctp/associola.c:997
 sctp_inq_push+0x23e/0x2b0 net/sctp/inqueue.c:80
 sctp_backlog_rcv+0x394/0xd80 net/sctp/input.c:331
 sk_backlog_rcv include/net/sock.h:1115 [inline]
 __release_sock+0x207/0x570 net/core/sock.c:2911
 release_sock+0x6b/0x1e0 net/core/sock.c:3478
 sctp_wait_for_connect+0x486/0x810 net/sctp/socket.c:9325
 sctp_sendmsg_to_asoc+0x1ea7/0x1ee0 net/sctp/socket.c:1884
 ...

Uninit was created at:
 slab_post_alloc_hook+0x12d/0xb60 mm/slab.h:716
 slab_alloc_node mm/slub.c:3451 [inline]
 __kmem_cache_alloc_node+0x4ff/0x8b0 mm/slub.c:3490
 __do_kmalloc_node mm/slab_common.c:965 [inline]
 __kmalloc_node_track_caller+0x118/0x3c0 mm/slab_common.c:986
 kmalloc_reserve+0x248/0x470 net/core/skbuff.c:585
 __alloc_skb+0x318/0x740 net/core/skbuff.c:654
 alloc_skb include/linux/skbuff.h:1288 [inline]
 sctp_packet_pack net/sctp/output.c:472 [inline]
 sctp_packet_transmit+0x1729/0x4150 net/sctp/output.c:621
 sctp_outq_flush_transports net/sctp/outqueue.c:1173 [inline]
 ...

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-and-tested-by: syzbot+70a42f45e76bede082be@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=70a42f45e76bede082be
Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
---
 net/sctp/inqueue.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/sctp/inqueue.c b/net/sctp/inqueue.c
index 7182c5a450fb..98ce9524c87c 100644
--- a/net/sctp/inqueue.c
+++ b/net/sctp/inqueue.c
@@ -197,6 +197,7 @@ struct sctp_chunk *sctp_inq_pop(struct sctp_inq *queue)
 		}
 	}
 
+	ch->length = htons(sizeof(*ch));
 	chunk->chunk_hdr = ch;
 	chunk->chunk_end = ((__u8 *)ch) + SCTP_PAD4(ntohs(ch->length));
 	skb_pull(chunk->skb, sizeof(*ch));
-- 
2.25.1

Re: [PATCH net] sctp: fix uninit-value in sctp_inq_pop()

Posted by kernel test robot 2 years, 3 months ago


Hello,

kernel test robot noticed "stress-ng.sctp.fail" on:

commit: 8c7dea932ab60ed7519996ade2d1e21db2c76cd4 ("[PATCH net] sctp: fix uninit-value in sctp_inq_pop()")
url: https://github.com/intel-lab-lkp/linux/commits/Nikita-Zhandarovich/sctp-fix-uninit-value-in-sctp_inq_pop/20230829-151540
base: https://git.kernel.org/cgit/linux/kernel/git/davem/net.git 90ca51e8c654699b672ba61aeaa418dfb3252e5e
patch link: https://lore.kernel.org/all/20230829071334.58083-1-n.zhandarovich@fintech.ru/
patch subject: [PATCH net] sctp: fix uninit-value in sctp_inq_pop()

in testcase: stress-ng
version: stress-ng-x86_64-0.15.04-1_20230812
with following parameters:

	nr_threads: 100%
	testtime: 60s
	class: network
	test: sctp
	cpufreq_governor: performance



compiler: gcc-12
test machine: 64 threads 2 sockets Intel(R) Xeon(R) Gold 6346 CPU @ 3.10GHz (Ice Lake) with 256G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)




If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202309081012.be01cf11-oliver.sang@intel.com

2023-09-07 16:33:40 stress-ng --timeout 60 --times --verify --metrics-brief --sctp 64
stress-ng: info:  [4148] setting to a 60 second run per stressor
stress-ng: info:  [4148] dispatching hogs: 64 sctp
stress-ng: info:  [4148] stressor       bogo ops real time  usr time  sys time   bogo ops/s     bogo ops/s
stress-ng: info:  [4148]                           (secs)    (secs)    (secs)   (real time) (usr+sys time)
stress-ng: info:  [4148] sctp                  0     60.00      0.03      0.01         0.00           0.00
stress-ng: warn:  [4148] metrics-check: all bogo-op counters are zero, data may be incorrect
stress-ng: info:  [4148] for a 60.01s run time:
stress-ng: info:  [4148]    3840.71s available CPU time
stress-ng: info:  [4148]       0.04s user time   (  0.00%)
stress-ng: info:  [4148]       0.01s system time (  0.00%)
stress-ng: info:  [4148]       0.05s total time  (  0.00%)
stress-ng: info:  [4148] load average: 0.63 0.32 0.12
stress-ng: info:  [4148] successful run completed in 60.01s (1 min, 0.01 secs)



The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20230908/202309081012.be01cf11-oliver.sang@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net] sctp: fix uninit-value in sctp_inq_pop()

Posted by Xin Long 2 years, 3 months ago

On Tue, Aug 29, 2023 at 3:14 AM Nikita Zhandarovich
<n.zhandarovich@fintech.ru> wrote:
>
> Syzbot identified a case [1] of uninitialized memory usage in
> sctp_inq_pop(), specifically in 'ch->length'.
>
> Fix the issue by ensuring that 'ch->length' reflects the size of
> 'sctp_chunkhdr *ch' before accessing it.
>
> [1]
> BUG: KMSAN: uninit-value in sctp_inq_pop+0x1597/0x1910 net/sctp/inqueue.c:205
>  sctp_inq_pop+0x1597/0x1910 net/sctp/inqueue.c:205
>  sctp_assoc_bh_rcv+0x1a7/0xc50 net/sctp/associola.c:997
>  sctp_inq_push+0x23e/0x2b0 net/sctp/inqueue.c:80
>  sctp_backlog_rcv+0x394/0xd80 net/sctp/input.c:331
>  sk_backlog_rcv include/net/sock.h:1115 [inline]
>  __release_sock+0x207/0x570 net/core/sock.c:2911
>  release_sock+0x6b/0x1e0 net/core/sock.c:3478
>  sctp_wait_for_connect+0x486/0x810 net/sctp/socket.c:9325
>  sctp_sendmsg_to_asoc+0x1ea7/0x1ee0 net/sctp/socket.c:1884
>  ...
>
> Uninit was stored to memory at:
>  sctp_inq_pop+0x151a/0x1910 net/sctp/inqueue.c:201
>  sctp_assoc_bh_rcv+0x1a7/0xc50 net/sctp/associola.c:997
>  sctp_inq_push+0x23e/0x2b0 net/sctp/inqueue.c:80
>  sctp_backlog_rcv+0x394/0xd80 net/sctp/input.c:331
>  sk_backlog_rcv include/net/sock.h:1115 [inline]
>  __release_sock+0x207/0x570 net/core/sock.c:2911
>  release_sock+0x6b/0x1e0 net/core/sock.c:3478
>  sctp_wait_for_connect+0x486/0x810 net/sctp/socket.c:9325
>  sctp_sendmsg_to_asoc+0x1ea7/0x1ee0 net/sctp/socket.c:1884
>  ...
>
> Uninit was created at:
>  slab_post_alloc_hook+0x12d/0xb60 mm/slab.h:716
>  slab_alloc_node mm/slub.c:3451 [inline]
>  __kmem_cache_alloc_node+0x4ff/0x8b0 mm/slub.c:3490
>  __do_kmalloc_node mm/slab_common.c:965 [inline]
>  __kmalloc_node_track_caller+0x118/0x3c0 mm/slab_common.c:986
>  kmalloc_reserve+0x248/0x470 net/core/skbuff.c:585
>  __alloc_skb+0x318/0x740 net/core/skbuff.c:654
>  alloc_skb include/linux/skbuff.h:1288 [inline]
>  sctp_packet_pack net/sctp/output.c:472 [inline]
>  sctp_packet_transmit+0x1729/0x4150 net/sctp/output.c:621
>  sctp_outq_flush_transports net/sctp/outqueue.c:1173 [inline]
>  ...
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-and-tested-by: syzbot+70a42f45e76bede082be@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=70a42f45e76bede082be
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> ---
>  net/sctp/inqueue.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/net/sctp/inqueue.c b/net/sctp/inqueue.c
> index 7182c5a450fb..98ce9524c87c 100644
> --- a/net/sctp/inqueue.c
> +++ b/net/sctp/inqueue.c
> @@ -197,6 +197,7 @@ struct sctp_chunk *sctp_inq_pop(struct sctp_inq *queue)
>                 }
>         }
>
> +       ch->length = htons(sizeof(*ch));
>         chunk->chunk_hdr = ch;
>         chunk->chunk_end = ((__u8 *)ch) + SCTP_PAD4(ntohs(ch->length));
>         skb_pull(chunk->skb, sizeof(*ch));
> --
> 2.25.1
>
Hi, Nikita

You can't just overwrite "ch->length", "ch" is the header of the received chunk.
if it says ch->length is Uninit, it means either the chunk parsing in
the receiver
is overflow or the format of the chunk created in the sender is incorrect.

If you can reproduce it stably, I suggest you start from sctp_inq_pop() and
print out the skb info and data in there, and see if it's a normal chunk.

Thanks.

Re: [PATCH net] sctp: fix uninit-value in sctp_inq_pop()

Posted by Nikita Zhandarovich 2 years, 3 months ago


On 8/29/23 13:27, Xin Long wrote:
> On Tue, Aug 29, 2023 at 3:14 AM Nikita Zhandarovich
> <n.zhandarovich@fintech.ru> wrote:
>>
>> Syzbot identified a case [1] of uninitialized memory usage in
>> sctp_inq_pop(), specifically in 'ch->length'.
>>
>> Fix the issue by ensuring that 'ch->length' reflects the size of
>> 'sctp_chunkhdr *ch' before accessing it.
>>
>> [1]
>> BUG: KMSAN: uninit-value in sctp_inq_pop+0x1597/0x1910 net/sctp/inqueue.c:205
>>  sctp_inq_pop+0x1597/0x1910 net/sctp/inqueue.c:205
>>  sctp_assoc_bh_rcv+0x1a7/0xc50 net/sctp/associola.c:997
>>  sctp_inq_push+0x23e/0x2b0 net/sctp/inqueue.c:80
>>  sctp_backlog_rcv+0x394/0xd80 net/sctp/input.c:331
>>  sk_backlog_rcv include/net/sock.h:1115 [inline]
>>  __release_sock+0x207/0x570 net/core/sock.c:2911
>>  release_sock+0x6b/0x1e0 net/core/sock.c:3478
>>  sctp_wait_for_connect+0x486/0x810 net/sctp/socket.c:9325
>>  sctp_sendmsg_to_asoc+0x1ea7/0x1ee0 net/sctp/socket.c:1884
>>  ...
>>
>> Uninit was stored to memory at:
>>  sctp_inq_pop+0x151a/0x1910 net/sctp/inqueue.c:201
>>  sctp_assoc_bh_rcv+0x1a7/0xc50 net/sctp/associola.c:997
>>  sctp_inq_push+0x23e/0x2b0 net/sctp/inqueue.c:80
>>  sctp_backlog_rcv+0x394/0xd80 net/sctp/input.c:331
>>  sk_backlog_rcv include/net/sock.h:1115 [inline]
>>  __release_sock+0x207/0x570 net/core/sock.c:2911
>>  release_sock+0x6b/0x1e0 net/core/sock.c:3478
>>  sctp_wait_for_connect+0x486/0x810 net/sctp/socket.c:9325
>>  sctp_sendmsg_to_asoc+0x1ea7/0x1ee0 net/sctp/socket.c:1884
>>  ...
>>
>> Uninit was created at:
>>  slab_post_alloc_hook+0x12d/0xb60 mm/slab.h:716
>>  slab_alloc_node mm/slub.c:3451 [inline]
>>  __kmem_cache_alloc_node+0x4ff/0x8b0 mm/slub.c:3490
>>  __do_kmalloc_node mm/slab_common.c:965 [inline]
>>  __kmalloc_node_track_caller+0x118/0x3c0 mm/slab_common.c:986
>>  kmalloc_reserve+0x248/0x470 net/core/skbuff.c:585
>>  __alloc_skb+0x318/0x740 net/core/skbuff.c:654
>>  alloc_skb include/linux/skbuff.h:1288 [inline]
>>  sctp_packet_pack net/sctp/output.c:472 [inline]
>>  sctp_packet_transmit+0x1729/0x4150 net/sctp/output.c:621
>>  sctp_outq_flush_transports net/sctp/outqueue.c:1173 [inline]
>>  ...
>>
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Reported-and-tested-by: syzbot+70a42f45e76bede082be@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=70a42f45e76bede082be
>> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
>> ---
>>  net/sctp/inqueue.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/net/sctp/inqueue.c b/net/sctp/inqueue.c
>> index 7182c5a450fb..98ce9524c87c 100644
>> --- a/net/sctp/inqueue.c
>> +++ b/net/sctp/inqueue.c
>> @@ -197,6 +197,7 @@ struct sctp_chunk *sctp_inq_pop(struct sctp_inq *queue)
>>                 }
>>         }
>>
>> +       ch->length = htons(sizeof(*ch));
>>         chunk->chunk_hdr = ch;
>>         chunk->chunk_end = ((__u8 *)ch) + SCTP_PAD4(ntohs(ch->length));
>>         skb_pull(chunk->skb, sizeof(*ch));
>> --
>> 2.25.1
>>
> Hi, Nikita
> 
> You can't just overwrite "ch->length", "ch" is the header of the received chunk.
> if it says ch->length is Uninit, it means either the chunk parsing in
> the receiver
> is overflow or the format of the chunk created in the sender is incorrect.
> 
> If you can reproduce it stably, I suggest you start from sctp_inq_pop() and
> print out the skb info and data in there, and see if it's a normal chunk.
> 
> Thanks.

Thank you for your feedback, I'll follow your advice and try to narrow
the problem down.

With regards,
Nikita