linux-kernel - 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-prev] [day] [month] [year] [list]

Message-ID: <01100196b0157e76-acca71aa-f8d0-45b0-bb09-de38e1bfa341-000000@eu-north-1.amazonses.com>
Date: Thu, 8 May 2025 13:28:40 +0000
From: Ozgur Kara <ozgur@...sey.org>
To: Neal Cardwell <ncardwell@...gle.com>
Cc: Ozgur Kara <ozgur@...sey.org>, 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

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