linux-kernel - Re: [PATCH net] netlink: check for null extack in cookie helpers

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: <b4b1d7b252820591ebb00e3851d44dc6c3f2d1b9.camel@sipsolutions.net>
Date:   Fri, 20 Mar 2020 22:22:45 +0100
From:   Johannes Berg <johannes@...solutions.net>
To:     Michal Kubecek <mkubecek@...e.cz>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] netlink: check for null extack in cookie helpers

Hi Michal,

> Unlike NL_SET_ERR_* macros, nl_set_extack_cookie_u64() and
> nl_set_extack_cookie_u32() helpers do not check extack argument for null
> and neither do their callers, as syzbot recently discovered for
> ethnl_parse_header().

What exactly did it discover?

> Instead of fixing the callers and leaving the trap in place, add check of
> null extack to both helpers to make them consistent with NL_SET_ERR_*
> macros.
> 
> Fixes: 2363d73a2f3e ("ethtool: reject unrecognized request flags")
> Fixes: 9bb7e0f24e7e ("cfg80211: add peer measurement with FTM initiator API")

I'm not really convinced, at least not for the second patch.

After all, this is an important part of the functionality, and the whole
thing is pretty useless if no extack/cookie is returned since then you
don't have a handle to the in-progress operation.

That was the intention originally too, until now the cookie also got
used for auxiliary error information...

Now, I don't think we need to *crash* when something went wrong here,
but then I'd argue there should at least be a WARN_ON(). But then that
means syzbot will just trigger the WARN_ON which also makes it unhappy,
so you still would have to check in the caller?

johannes