Message-ID: <CANn89iKeafqV+pTptNZtEsjNchRSxe2mC7FOaWtwXNMaXjzcPQ@mail.gmail.com>
Date: Thu, 8 May 2025 08:02:27 -0700
From: Eric Dumazet <edumazet@...gle.com>
To: Ozgur Kara <ozgur@...sey.org>
Cc: Neal Cardwell <ncardwell@...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:28 AM Ozgur Kara <ozgur@...sey.org> wrote:
>
> Neal Cardwell <ncardwell@...gle.com>, 8 May 2025 Per, 15:54 tarihinde
> şunu yazdı:
> >
> > 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
> >
>
> Hello Neal, I will pay attention to this sorry.
>
> > > ---
> > >  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.
> >
>
> The issue is more nuanced than opt being only NULL, while opt =
> &TCP_SKB_CB(skb)->header.h4.opt gives a valid pointer to a structure
> and the contents of that structure might be uninitialized or invalid
> in certain code paths.

It must not.

TCP stack is called after IPv4 traversal.

We are not going to add in TCP defensive code.

Instead, if you think there is a bug in the way IPv4 options are
decoded (before reaching TCP),
please fix it in the correct layer.

Thanks.

> My patch adds defensive programming by checking three conditions
> before accessing opt->faddr: whether opt itself is valid, opt->optlen
> is non-zero and opt->srr is set.
> This prevents undefined behavior when accessing opt->faddr in cases
> where the structure's fields haven't been properly initialized.
>
> The previous code (opt->srr ? opt->faddr : ireq->ir_rmt_addr) assumed
> opt->srr was always valid, while the new code safely establishes daddr
> = ireq->ir_rmt_addr as the default, only using opt->faddr when all
> safety conditions are met.
>  However, the issue lies in the validity of the struct ip_options
> content, particularly opt->srr and opt->faddr. If the
> TCP_SKB_CB(skb)->header.h4.opt structure is uninitialized or reset
> (e.g., via memset or incomplete parsing), fields like opt->optlen and
> opt->srr may contain garbage values, leading to undefined behavior
> when accessed.
>
> A specific example of this vulnerability occurs during early SYN
> transactions, particularly if tcp_v4_cookie_check() is called
> directly.

How 'directly' is this done ? Are you talking about an out-of-tree code ?

 In this scenario, opt->optlen might be zero while other
> fields contain garbage values, which could lead to memory corruption
> or security issues.
> So this patch ensures robustness against invalid options, especially
> in edge cases like malformed SYN packets, with minimal overhead.
>
> am i mistaken? if there is missing information please forward it.
>
> Regards
>
> Ozgur
>
> > thanks,
> > neal
> >
> >

Powered by blists - more mailing lists