Re: [Linux-kernel-mentees] [PATCH] net: tipc: fix general protection fault in tipc_conn_delete_sub

From: Ying Xue <ying.xue@windriver.com>
To: Greg KH <gregkh@linuxfoundation.org>,
	B K Karthik <bkkarthik@pesu.pes.edu>
Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Jon Maloy <jmaloy@redhat.com>,
	tipc-discussion@lists.sourceforge.net,
	Jakub Kicinski <kuba@kernel.org>,
	linux-kernel-mentees@lists.linuxfoundation.org,
	"David S. Miller" <davem@davemloft.net>
Subject: Re: [Linux-kernel-mentees] [PATCH] net: tipc: fix general protection fault in tipc_conn_delete_sub
Date: Wed, 29 Jul 2020 00:15:25 +0800	[thread overview]
Message-ID: <6d6197ba-ce38-09ec-78a1-7feb59cd7a90@windriver.com> (raw)
In-Reply-To: <20200727142416.GA186956@kroah.com>

On 7/27/20 10:24 PM, Greg KH wrote:
>>>> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
>>>> index 1489cfb941d8..6c8d0c6bb112 100644
>>>> --- a/net/tipc/topsrv.c
>>>> +++ b/net/tipc/topsrv.c
>>>> @@ -255,6 +255,9 @@ static void tipc_conn_send_to_sock(struct tipc_conn *con)
>>>>       int count = 0;
>>>>       int ret;
>>>>
>>>> +     if (!con->server)
>>>> +             return -EINVAL;
>>> What is wrong with looking at the srv local variable instead?
>>>
>>> And how is server getting set to NULL and this function still being
>>> called?
>> tipc_conn_send_work makes a call to connected() which just returns con
>> && test_bit(CF_CONNECTED, &con->flags)
>> maybe we can add this check to the implementation of connection() if
>> you agree, but I found this solution to be fairly simpler because I'm
>> not sure where else connected() is being used, and I did not want to
>> introduce redundant function calls.
> That's not what I asked here at all
I agreed with Greg. The key problem is that we need to understand why
con->server got NULL, otherwise, we probably just hide the issue by
checking if it's NULL.

The topology server is created in kernel space as an internal TIPC
server and its life cycle is the same as TIPC network namespace.
Whenever the topology server accepts a connection from its client, it
will create a "con" which will be used to talk to the client and the
topology server instance (ie, "topsrv") will be attached to
"con->server". In theory, "con" cannot be died before its server, as a
result, con->server cannot become NULL in tipc_conn_send_to_sock().

So I suspect there is other potential issues which caused this problem,
for example, the refcount of "con" is not properly taken or put and this
case is triggered before of use-after-free, or race condition etc.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees