kcm: Fix kernel NULL pointer dereference in requeue_rx_msgs

[PATCH net] kcm: Fix kernel NULL pointer dereference in requeue_rx_msgs

Posted by Wang Hai 3 years, 5 months ago

In kcm_rcv_strparser(), the skb is queued to the kcm that is currently
being reserved, and if the queue is full, unreserve_rx_kcm() will be
called. At this point, if KCM_RECV_DISABLE is set, then unreserve_rx_kcm()
will requeue received messages for the current kcm socket to other kcm
sockets. The kcm sock lock is not held during this time, and as long as
someone calls kcm_recvmsg, it will concurrently unlink the same skb, which
ill result in a null pointer reference.

cpu0 			cpu1		        cpu2
kcm_rcv_strparser
 reserve_rx_kcm
                        kcm_setsockopt
                         kcm_recv_disable
                          kcm->rx_disabled = 1;
  kcm_queue_rcv_skb
  unreserve_rx_kcm
   requeue_rx_msgs                              kcm_recvmsg
    __skb_dequeue
     __skb_unlink(skb)				  skb_unlink(skb)
                                                  //double unlink skb

There is no need to re-queue the received msg to other kcm when unreserve
kcm. Remove it.

The following is the error log:

BUG: kernel NULL pointer dereference, address: 0000000000000008
...
RIP: 0010:skb_unlink+0x40/0x50
...
Call Trace:
 kcm_recvmsg+0x143/0x180
 ____sys_recvmsg+0x16a/0x180
 ___sys_recvmsg+0x80/0xc0
 do_recvmmsg+0xc2/0x2a0
 __sys_recvmmsg+0x10c/0x160
 __x64_sys_recvmmsg+0x25/0x40
 do_syscall_64+0x3b/0x90
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

Fixes: ab7ac4eb9832 ("kcm: Kernel Connection Multiplexor module")
Signed-off-by: Wang Hai <wanghai38@huawei.com>
---
 net/kcm/kcmsock.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
index a5004228111d..691d40364b8f 100644
--- a/net/kcm/kcmsock.c
+++ b/net/kcm/kcmsock.c
@@ -333,9 +333,8 @@ static void unreserve_rx_kcm(struct kcm_psock *psock,
 		return;
 	}
 
-	if (unlikely(kcm->rx_disabled)) {
-		requeue_rx_msgs(mux, &kcm->sk.sk_receive_queue);
-	} else if (rcv_ready || unlikely(!sk_rmem_alloc_get(&kcm->sk))) {
+	if (likely(!kcm->rx_disabled) &&
+	    (rcv_ready || unlikely(!sk_rmem_alloc_get(&kcm->sk)))) {
 		/* Check for degenerative race with rx_wait that all
 		 * data was dequeued (accounted for in kcm_rfree).
 		 */
-- 
2.17.1

Re: [PATCH net] kcm: Fix kernel NULL pointer dereference in requeue_rx_msgs

Posted by Cong Wang 3 years, 5 months ago

On Sat, Nov 12, 2022 at 08:04:23PM +0800, Wang Hai wrote:
> In kcm_rcv_strparser(), the skb is queued to the kcm that is currently
> being reserved, and if the queue is full, unreserve_rx_kcm() will be
> called. At this point, if KCM_RECV_DISABLE is set, then unreserve_rx_kcm()
> will requeue received messages for the current kcm socket to other kcm
> sockets. The kcm sock lock is not held during this time, and as long as
> someone calls kcm_recvmsg, it will concurrently unlink the same skb, which
> ill result in a null pointer reference.
> 
> cpu0 			cpu1		        cpu2
> kcm_rcv_strparser
>  reserve_rx_kcm
>                         kcm_setsockopt
>                          kcm_recv_disable
>                           kcm->rx_disabled = 1;
>   kcm_queue_rcv_skb
>   unreserve_rx_kcm
>    requeue_rx_msgs                              kcm_recvmsg
>     __skb_dequeue
>      __skb_unlink(skb)				  skb_unlink(skb)
>                                                   //double unlink skb
> 

We will hold skb queue lock after my patch, so this will not happen after
applying my patch below?
https://lore.kernel.org/netdev/20221114005119.597905-1-xiyou.wangcong@gmail.com/

Thanks.

Re: [PATCH net] kcm: Fix kernel NULL pointer dereference in requeue_rx_msgs

Posted by wanghai (M) 3 years, 5 months ago

在 2022/11/14 8:55, Cong Wang 写道:
> On Sat, Nov 12, 2022 at 08:04:23PM +0800, Wang Hai wrote:
>> In kcm_rcv_strparser(), the skb is queued to the kcm that is currently
>> being reserved, and if the queue is full, unreserve_rx_kcm() will be
>> called. At this point, if KCM_RECV_DISABLE is set, then unreserve_rx_kcm()
>> will requeue received messages for the current kcm socket to other kcm
>> sockets. The kcm sock lock is not held during this time, and as long as
>> someone calls kcm_recvmsg, it will concurrently unlink the same skb, which
>> ill result in a null pointer reference.
>>
>> cpu0 			cpu1		        cpu2
>> kcm_rcv_strparser
>>   reserve_rx_kcm
>>                          kcm_setsockopt
>>                           kcm_recv_disable
>>                            kcm->rx_disabled = 1;
>>    kcm_queue_rcv_skb
>>    unreserve_rx_kcm
>>     requeue_rx_msgs                              kcm_recvmsg
>>      __skb_dequeue
>>       __skb_unlink(skb)				  skb_unlink(skb)
>>                                                    //double unlink skb
>>
> We will hold skb queue lock after my patch, so this will not happen after
> applying my patch below?
> https://lore.kernel.org/netdev/20221114005119.597905-1-xiyou.wangcong@gmail.com/

Hi Cong,

I tested your patch and it fixed my problem, thanks.

> .

-- 
Wang Hai