netdev - Re: [PATCH net] ipmr: fix kernel panic when forwarding mcast packets

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: <CANn89iJkEFuvYMHjM6g=4Jc2mp4wW1rN10QwxxvyfOJYC2h8mQ@mail.gmail.com>
Date: Wed, 24 Jan 2024 15:21:00 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Nicolas Dichtel <nicolas.dichtel@...nd.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Leone Fernando <leone4fernando@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net] ipmr: fix kernel panic when forwarding mcast packets

On Wed, Jan 24, 2024 at 1:15 PM Nicolas Dichtel
<nicolas.dichtel@...nd.com> wrote:
>
> The stacktrace was:
> [   86.305548] BUG: kernel NULL pointer dereference, address: 0000000000000092
>
...

> The original packet in ipmr_cache_report() may be queued and then forwarded
> with ip_mr_forward(). This last function has the assumption that the skb
> dst is set.
>
> After the below commit, the skb dst is dropped by ipv4_pktinfo_prepare(),
> which causes the oops.
>
> Fixes: bb7403655b3c ("ipmr: support IP_PKTINFO on cache report IGMP msg")
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@...nd.com>
> ---
>  include/net/ip.h       | 2 +-
>  net/ipv4/ip_sockglue.c | 5 +++--
>  net/ipv4/ipmr.c        | 2 +-
>  net/ipv4/raw.c         | 2 +-
>  net/ipv4/udp.c         | 2 +-
>  5 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/ip.h b/include/net/ip.h
> index de0c69c57e3c..1e7f2e417ed2 100644
> --- a/include/net/ip.h
> +++ b/include/net/ip.h
> @@ -767,7 +767,7 @@ int ip_options_rcv_srr(struct sk_buff *skb, struct net_device *dev);
>   *     Functions provided by ip_sockglue.c
>   */
>
> -void ipv4_pktinfo_prepare(const struct sock *sk, struct sk_buff *skb);
> +void ipv4_pktinfo_prepare(const struct sock *sk, struct sk_buff *skb, bool keep_dst);
>  void ip_cmsg_recv_offset(struct msghdr *msg, struct sock *sk,
>                          struct sk_buff *skb, int tlen, int offset);
>  int ip_cmsg_send(struct sock *sk, struct msghdr *msg,
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index 7aa9dc0e6760..fe1ab335324f 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -1368,7 +1368,7 @@ int do_ip_setsockopt(struct sock *sk, int level, int optname,
>   * destination in skb->cb[] before dst drop.
>   * This way, receiver doesn't make cache line misses to read rtable.
>   */
> -void ipv4_pktinfo_prepare(const struct sock *sk, struct sk_buff *skb)
> +void ipv4_pktinfo_prepare(const struct sock *sk, struct sk_buff *skb, bool keep_dst)
>  {
>         struct in_pktinfo *pktinfo = PKTINFO_SKB_CB(skb);
>         bool prepare = inet_test_bit(PKTINFO, sk) ||
> @@ -1397,7 +1397,8 @@ void ipv4_pktinfo_prepare(const struct sock *sk, struct sk_buff *skb)
>                 pktinfo->ipi_ifindex = 0;
>                 pktinfo->ipi_spec_dst.s_addr = 0;
>         }
> -       skb_dst_drop(skb);
> +       if (keep_dst == false)
> +               skb_dst_drop(skb);

IMO this would look nicer if you had

void ipv4_pktinfo_prepare(const struct sock *sk, struct sk_buff *skb,
bool drop_dst)
..
if (drop_dst)
   skb_dst_drop(skb);

Reviewed-by: Eric Dumazet <edumazet@...gle.com>

Thanks.