Re: [PATCH] don't assume the existence of skb->dev when trying to reset ip_options in ipv4_send_dest_unreach

[Kyle Zeng <zengyhkyle@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2098 bytes --]

On Thu, Sep 07, 2023 at 01:03:52PM +0200, Paolo Abeni wrote:
> On Wed, 2023-09-06 at 19:43 -0700, Kyle Zeng wrote:
> > Currently, we assume the skb is associated with a device before calling __ip_options_compile, which is not always the case if it is re-routed by ipvs.
> > When skb->dev is NULL, dev_net(skb->dev) will become null-dereference.
> > Since we know that all the options will be set to IPOPT_END, which does
> > not depend on struct net, we pass NULL to it.
> 
> It's not clear to me why we can infer the above. Possibly would be more
> safe to skip entirely the __ip_options_compile() call?!?
> 
> Please at least clarify the changelog and trim it to 72 chars. 
> 
> Additionally trim the subj to the same len and include the target tree
> (net) into the subj prefix.
> 
> Thanks!
> 
> Paolo
> 

Hi Paolo,

> It's not clear to me why we can infer the above. Possibly would be more
> safe to skip entirely the __ip_options_compile() call?!?
Sorry, after you pointed it out, I realized that I misunderstood the
code. Initially I thought `memset(&opt, 0, sizeof(opt));` would reset all
the option to OPOPT_END. But after carefully reading the code, it seems
that it only resets the io_options struct and the `optptr` is still the
original one. 

Do you think it is better to do:
`struct net = skb->dev ? dev_net(skb->dev) : NULL` ?

> Please at least clarify the changelog and trim it to 72 chars. 
> 
> Additionally trim the subj to the same len and include the target tree
> (net) into the subj prefix.
Sorry for that. I'm new to the Linux kernel community and I wonder whether
I should initiate a different patch or send another patch in this thread
in this case.

Hi David,

> ipv4_send_dest_unreach is called from ipv4_link_failure which might have
> an rtable (dst_entry) which has a device which is in a net namespace.
> That is better than blindly ignoring the namepsace.
Following your suggestion, I drafted another patch which is attached to
this email. I verified that the crash does not happen anymore. Can you
please advise whether it is a correct patch?

Thanks,
Kyle Zeng

[-- Attachment #2: 0001-fix-null-deref-in-ipv4_link_failure.patch --]
[-- Type: text/x-diff, Size: 1590 bytes --]

From ddf42a72bd2aabc7b66529ddadd90df420a73610 Mon Sep 17 00:00:00 2001
From: Kyle Zeng <zengyhkyle@gmail.com>
Date: Thu, 7 Sep 2023 15:49:46 -0700
Subject: [PATCH] fix null-deref in ipv4_link_failure

Currently, we assume the skb is associated with a device before calling
__ip_options_compile, which is not always the case if it is re-routed by
ipvs.
When skb->dev is NULL, dev_net(skb->dev) will become null-dereference.
This patch adds a check for the edge case and switch to use the net_device
from the rtable when skb->dev is NULL.

Suggested-by: Paolo Abeni<pabeni@redhat.com>
Suggested-by: David Ahern <dsahern@kernel.org>
Signed-off-by: Kyle Zeng <zengyhkyle@gmail.com>
Cc: Stephen Suryaputra <ssuryaextr@gmail.com>
---
 net/ipv4/route.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index d8c99bdc617..735a491e1ff 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1215,6 +1215,7 @@ static void ipv4_send_dest_unreach(struct sk_buff *skb)
 {
 	struct ip_options opt;
 	int res;
+	struct net_device *dev;
 
 	/* Recompile ip options since IPCB may not be valid anymore.
 	 * Also check we have a reasonable ipv4 header.
@@ -1230,7 +1231,8 @@ static void ipv4_send_dest_unreach(struct sk_buff *skb)
 		opt.optlen = ip_hdr(skb)->ihl * 4 - sizeof(struct iphdr);
 
 		rcu_read_lock();
-		res = __ip_options_compile(dev_net(skb->dev), &opt, skb, NULL);
+		dev = skb->dev ? skb->dev : skb_rtable(skb)->dst.dev;
+		res = __ip_options_compile(dev_net(net), &opt, skb, NULL);
 		rcu_read_unlock();
 
 		if (res)
-- 
2.34.1