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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZPpUfm/HhFet3ejH@westworld>
Date: Thu, 7 Sep 2023 15:53:50 -0700
From: Kyle Zeng <zengyhkyle@...il.com>
To: Paolo Abeni <pabeni@...hat.com>, dsahern@...nel.org
Cc: davem@...emloft.net, netdev@...r.kernel.org, ssuryaextr@...il.com
Subject: Re: [PATCH] don't assume the existence of skb->dev when trying to
 reset ip_options in ipv4_send_dest_unreach

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

View attachment "0001-fix-null-deref-in-ipv4_link_failure.patch" of type "text/x-diff" (1592 bytes)