netdev - Re: [PATCH] net: ipv4: Fix destination address determination in flowi4_init

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADVnQykrenhejQCcsNE6JBsk3bE5_rNTeQe3izrZd9qp8zmkYg@mail.gmail.com>
Date: Thu, 8 May 2025 08:53:37 -0400
From: Neal Cardwell <ncardwell@...gle.com>
To: Ozgur Kara <ozgur@...sey.org>
Cc: Eric Dumazet <edumazet@...gle.com>, "David S. Miller" <davem@...emloft.net>, 
	Kuniyuki Iwashima <kuniyu@...zon.com>, David Ahern <dsahern@...nel.org>, Jakub Kicinski <kuba@...nel.org>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, netdev@...r.kernel.org, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: ipv4: Fix destination address determination in flowi4_init_output

On Thu, May 8, 2025 at 6:21 AM Ozgur Kara <ozgur@...sey.org> wrote:
>
> From: Ozgur Karatas <ozgur@...sey.org>
>
> flowi4_init_output() function returns an argument and if opt->srr is
> true and opt->faddr is assigned to be checked before opt->faddr is
> used but if opt->srr seems to be true and opt->faddr is not set
> properly yet.
>
> opt itself will be an incompletely initialized struct and this access
> may cause a crash.
> * added daddr
> * like readability by passing a single daddr argument to
> flowi4_init_output() call.
>
> Signed-off-by: Ozgur Karatas <ozgur@...sey.org>

For bug fixes, please include a Fixes: footer; there are more details here:
   https://www.kernel.org/doc/html/v6.12/process/submitting-patches.html

> ---
>  net/ipv4/syncookies.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> index 5459a78b9809..2ff92d512825 100644
> --- a/net/ipv4/syncookies.c
> +++ b/net/ipv4/syncookies.c
> @@ -408,6 +408,7 @@ struct sock *cookie_v4_check(struct sock *sk,
> struct sk_buff *skb)
>         struct flowi4 fl4;
>         struct rtable *rt;
>         __u8 rcv_wscale;
> +       __be32 daddr;
>         int full_space;
>         SKB_DR(reason);
>
> @@ -442,6 +443,17 @@ struct sock *cookie_v4_check(struct sock *sk,
> struct sk_buff *skb)
>                 goto out_free;
>         }
>
> +        /* Safely determine destination address considered SRR option.
> +         * The flowi4 destination address is derived from opt->faddr
> if opt->srr is set.
> +         * However IP options are not always present in the skb and
> accessing opt->faddr
> +         * without validating opt->optlen and opt->srr can lead to
> undefined behavior.
> +         */
> +        if (opt && opt->optlen && opt->srr) {
> +                daddr = opt->faddr;
> +        } else {
> +                daddr = ireq->ir_rmt_addr;
> +        }

Can you please explain how opt could be NULL, given how it is
initialized, like this:
        struct ip_options *opt = &TCP_SKB_CB(skb)->header.h4.opt;
?

And can you please explain how opt->srr could be set if opt->optlen is
0? I'm not seeing how it's possible, given how the
__ip_options_compile() code is structured. But perhaps I am missing
something.

thanks,
neal