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]

Date: Thu, 7 Sep 2023 18:45:03 -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 7, 2023 at 3:53 PM Kyle Zeng <zengyhkyle@...il.com> wrote:
>
> 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

Sorry for the typo in the previous patch. I fixed it and tested it.
My proof-of-concept code can no longer trigger the crash.
The patch is attached to this email.

Thanks,
Kyle Zeng

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