Message-ID: <01100196b6b28830-bda13534-f814-44ee-93f7-1517626293ba-000000@eu-north-1.amazonses.com>
Date: Fri, 9 May 2025 20:17:55 +0000
From: Ozgur Kara <ozgur@...sey.org>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Ozgur Kara <ozgur@...sey.org>, 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

Eric Dumazet <edumazet@...gle.com>, 8 May 2025 Per, 18:02 tarihinde şunu yazdı:
>
> 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.
>

Hello Eric,

okay got it so i will look at IP layer and try to understand it.

> > 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 ?
>

actually yes, its in out-of-tree code because i thought it could be
for example LSM, bpf or any hook or handler (if a module is added to
kernel).
Maybe special socket operations, mechanisms such as tcp fast open can
be triggered before state is fully established.
In short, if someone calls function at wrong time syn cookie
verification will be done in the early state and in this case I guess
so state will be created or spoof syn will be turned on.

Maybe i should test this and look with csope, strace and reconsider.

Regards

Ozgur

>  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