Re: [PATCH] net: ipv4: Fix destination address determination in flowi4_init_output

Neal Cardwell [off-list ref], 8 May 2025 Per, 15:54 tarihinde
şunu yazdı:

On Thu, May 8, 2025 at 6:21 AM Ozgur Kara [off-list ref] wrote:

quoted

From: Ozgur Karatas <redacted>

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

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.

quoted

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