All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lee Jones <lee@kernel.org>
To: Eric Dumazet <edumazet@google.com>, stable@vger.kernel.org
Cc: "David S . Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	David Ahern <dsahern@kernel.org>,
	netdev@vger.kernel.org, eric.dumazet@gmail.com,
	Clement Lecigne <clecigne@google.com>,
	Tom Herbert <tom@herbertland.com>
Subject: Re: [PATCH net] net: fix __dst_negative_advice() race
Date: Wed, 12 Jun 2024 16:52:35 +0100	[thread overview]
Message-ID: <20240612155235.GA2187093@google.com> (raw)
In-Reply-To: <20240528114353.1794151-1-edumazet@google.com>

On Tue, 28 May 2024, Eric Dumazet wrote:

> __dst_negative_advice() does not enforce proper RCU rules when
> sk->dst_cache must be cleared, leading to possible UAF.
> 
> RCU rules are that we must first clear sk->sk_dst_cache,
> then call dst_release(old_dst).
> 
> Note that sk_dst_reset(sk) is implementing this protocol correctly,
> while __dst_negative_advice() uses the wrong order.
> 
> Given that ip6_negative_advice() has special logic
> against RTF_CACHE, this means each of the three ->negative_advice()
> existing methods must perform the sk_dst_reset() themselves.
> 
> Note the check against NULL dst is centralized in
> __dst_negative_advice(), there is no need to duplicate
> it in various callbacks.
> 
> Many thanks to Clement Lecigne for tracking this issue.
> 
> This old bug became visible after the blamed commit, using UDP sockets.
> 
> Fixes: a87cb3e48ee8 ("net: Facility to report route quality of connected sockets")
> Reported-by: Clement Lecigne <clecigne@google.com>
> Diagnosed-by: Clement Lecigne <clecigne@google.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Tom Herbert <tom@herbertland.com>
> ---
>  include/net/dst_ops.h  |  2 +-
>  include/net/sock.h     | 13 +++----------
>  net/ipv4/route.c       | 22 ++++++++--------------
>  net/ipv6/route.c       | 29 +++++++++++++++--------------
>  net/xfrm/xfrm_policy.c | 11 +++--------
>  5 files changed, 30 insertions(+), 47 deletions(-)

Could we have this patch in all Stable branches please?

Upstream commit:

  Fixes: 92f1655aa2b2 ("net: fix __dst_negative_advice() race")

-- 
Lee Jones [李琼斯]

      parent reply	other threads:[~2024-06-12 15:52 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-05-28 11:43 [PATCH net] net: fix __dst_negative_advice() race Eric Dumazet
2024-05-28 16:59 ` David Ahern
2024-05-30  1:20 ` patchwork-bot+netdevbpf
2024-06-12 15:52 ` Lee Jones [this message]

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=20240612155235.GA2187093@google.com \
    --to=lee@kernel.org \
    --cc=clecigne@google.com \
    --cc=davem@davemloft.net \
    --cc=dsahern@kernel.org \
    --cc=edumazet@google.com \
    --cc=eric.dumazet@gmail.com \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=stable@vger.kernel.org \
    --cc=tom@herbertland.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.