From: Mat Martineau <mathew.j.martineau@linux.intel.com>
To: Tim Gardner <tim.gardner@canonical.com>
Cc: mptcp@lists.linux.dev,
Matthieu Baerts <matthieu.baerts@tessares.net>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Paolo Abeni <pabeni@redhat.com>
Subject: Re: [PATCH][next] mptcp: Avoid NULL dereference in mptcp_getsockopt_subflow_addrs()
Date: Thu, 30 Sep 2021 16:14:36 -0700 (PDT) [thread overview]
Message-ID: <96389960-fd3-616d-c829-203d2414ffe@linux.intel.com> (raw)
In-Reply-To: <149982aa-720-35be-4866-39259b92884d@linux.intel.com>
On Mon, 20 Sep 2021, Mat Martineau wrote:
> On Mon, 20 Sep 2021, Tim Gardner wrote:
>
>> Coverity complains of a possible NULL dereference in
>> mptcp_getsockopt_subflow_addrs():
>>
>> 861 } else if (sk->sk_family == AF_INET6) {
>> 3. returned_null: inet6_sk returns NULL. [show details]
>> 4. var_assigned: Assigning: np = NULL return value from inet6_sk.
>> 862 const struct ipv6_pinfo *np = inet6_sk(sk);
>>
>> Fix this by checking for NULL.
>>
>> Cc: Mat Martineau <mathew.j.martineau@linux.intel.com>
>> Cc: Matthieu Baerts <matthieu.baerts@tessares.net>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: Jakub Kicinski <kuba@kernel.org>
>> Cc: netdev@vger.kernel.org
>> Cc: mptcp@lists.linux.dev
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
>>
>> [ I'm not at all sure this is the right thing to do since the final result is to
>> return garbage to user space in mptcp_getsockopt_subflow_addrs(). Is this one
>> of those cases where inet6_sk() can't fail ?]
>
> Hi Tim -
>
> Thanks for noticing this and proposing a fix.
>
> As you commented, this isn't the right change to merge since
> mptcp_getsockopt_subflow_addrs() would copy garbage.
>
> This block of code already checks that CONFIG_IPV6 is enabled, so the
> question is whether sk_fullsock() would return false because the subflow is
> in TCP_TIME_WAIT or TCP_NEW_SYN_RECV. The caller is iterating over sockets in
> the MPTCP socket's conn_list, which does not contain request_socks (so there
> are no sockets in the TCP_NEW_SYN_RECV state).
>
> TCP subflow sockets are normally removed from the conn_list before they are
> closed by their parent MPTCP socket, but I need to double-check for corner
> cases. I created a github issue to track this:
> https://github.com/multipath-tcp/mptcp_net-next/issues/231
>
Tim,
Could you submit a v2 of this patch? Paolo took a look and the condition
should not happen, but adding the NULL check would be a good idea and
returning early as your patch does is ok. The data copied after the early
return will be zeroed and look like the address family is AF_UNSPEC.
Could you add a
Fixes: https://github.com/multipath-tcp/mptcp_net-next/issues/231
tag and make the one change below?
>
>> ---
>> net/mptcp/sockopt.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
>> index 8137cc3a4296..c89f2bedce79 100644
>> --- a/net/mptcp/sockopt.c
>> +++ b/net/mptcp/sockopt.c
>> @@ -861,6 +861,9 @@ static void mptcp_get_sub_addrs(const struct sock *sk,
>> struct mptcp_subflow_addr
>> } else if (sk->sk_family == AF_INET6) {
>> const struct ipv6_pinfo *np = inet6_sk(sk);
>>
>> + if (!np)
This could be
if (WARN_ON_ONCE(!np))
(as suggested by Paolo) to make it clear the condition is unexpected.
>> + return;
>> +
>> a->sin6_local.sin6_family = AF_INET6;
>> a->sin6_local.sin6_port = inet->inet_sport;
>>
>> --
>> 2.33.0
Thanks,
--
Mat Martineau
Intel
next prev parent reply other threads:[~2021-09-30 23:14 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-20 15:42 [PATCH][next] mptcp: Avoid NULL dereference in mptcp_getsockopt_subflow_addrs() Tim Gardner
2021-09-21 0:05 ` Mat Martineau
2021-09-21 0:05 ` Mat Martineau
2021-09-30 23:14 ` Mat Martineau [this message]
2021-09-30 23:14 ` Mat Martineau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=96389960-fd3-616d-c829-203d2414ffe@linux.intel.com \
--to=mathew.j.martineau@linux.intel.com \
--cc=davem@davemloft.net \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=matthieu.baerts@tessares.net \
--cc=mptcp@lists.linux.dev \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=tim.gardner@canonical.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.