netdev - Re: [PATCH net,v4] netlink: fix potential sleeping issue in mqueue_flush

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <64ed7596ce8550079ecef8a76eaaacd594535e58.camel@redhat.com>
Date: Tue, 23 Jan 2024 11:48:17 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: shaozhengchao <shaozhengchao@...wei.com>, Pablo Neira Ayuso
	 <pablo@...filter.org>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com, 
 kuba@...nel.org, horms@...nel.org, anjali.k.kulkarni@...cle.com,
 kuniyu@...zon.com,  fw@...len.de, weiyongjun1@...wei.com,
 yuehaibing@...wei.com
Subject: Re: [PATCH net,v4] netlink: fix potential sleeping issue in
 mqueue_flush_file

Hi,

On Mon, 2024-01-22 at 19:10 +0800, shaozhengchao wrote:
> On 2024/1/22 16:56, Pablo Neira Ayuso wrote:
> > On Mon, Jan 22, 2024 at 09:18:07AM +0800, Zhengchao Shao wrote:
> > > I analyze the potential sleeping issue of the following processes:
> > > Thread A                                Thread B
> > > ...                                     netlink_create  //ref = 1
> > > do_mq_notify                            ...
> > >    sock = netlink_getsockbyfilp          ...     //ref = 2
> > >    info->notify_sock = sock;             ...
> > > ...                                     netlink_sendmsg
> > > ...                                       skb = netlink_alloc_large_skb  //skb->head is vmalloced
> > > ...                                       netlink_unicast
> > > ...                                         sk = netlink_getsockbyportid //ref = 3
> > > ...                                         netlink_sendskb
> > > ...                                           __netlink_sendskb
> > > ...                                             skb_queue_tail //put skb to sk_receive_queue
> > > ...                                         sock_put //ref = 2
> > > ...                                     ...
> > > ...                                     netlink_release
> > > ...                                       deferred_put_nlk_sk //ref = 1
> > > mqueue_flush_file
> > >    spin_lock
> > >    remove_notification
> > >      netlink_sendskb
> > >        sock_put  //ref = 0
> > >          sk_free
> > >            ...
> > >            __sk_destruct
> > >              netlink_sock_destruct
> > >                skb_queue_purge  //get skb from sk_receive_queue
> > >                  ...
> > >                  __skb_queue_purge_reason
> > >                    kfree_skb_reason
> > >                      __kfree_skb
> > >                      ...
> > >                      skb_release_all
> > >                        skb_release_head_state
> > >                          netlink_skb_destructor
> > >                            vfree(skb->head)  //sleeping while holding spinlock
> > > 
> > > In netlink_sendmsg, if the memory pointed to by skb->head is allocated by
> > > vmalloc, and is put to sk_receive_queue queue, also the skb is not freed.
> > > When the mqueue executes flush, the sleeping bug will occur. Use
> > > vfree_atomic instead of vfree in netlink_skb_destructor to solve the issue.
> > 
> > mqueue notification is of NOTIFY_COOKIE_LEN size:
> > 
> > static int do_mq_notify(mqd_t mqdes, const struct sigevent *notification)
> > {
> >          [...]
> >                  if (notification->sigev_notify == SIGEV_THREAD) {
> >                          long timeo;
> > 
> >                          /* create the notify skb */
> >                          nc = alloc_skb(NOTIFY_COOKIE_LEN, GFP_KERNEL);
> >                          if (!nc)
> >                                  return -ENOMEM;
> > 
> > Do you have a reproducer?
> Hi Pablo:
> 	I donot have reproducer. I found the issue when running syz on
> the 5.10 stable branch, but it only happened once. Then I analyzed the
> mainline code and found the same issue.
> 	The sock can be obtained from the value of sigev_signo
> transferred by the user in do_mq_notify. And the sock may be of type
> netlink, and it is possible to allocate the head area using vmalloc.
> Not only release the skb allocated in do_mq_notify, but also release
> the skb allocated in netlink_sendmsg when put sock mqueue_flush_file.
> What I missed?

I believe your explanation is solid and the patch looks safe, I'm
applying it.

Thank you!

Paolo

p.s. for once I'm answering in place of Pablo, which is sort of funny
given that our names are confused a significant number of times on the
ML...)