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

[Michal Kubecek <mkubecek@suse.cz>]

On Fri, Mar 20, 2020 at 10:22:45PM +0100, Johannes Berg wrote:
> 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?

It's this report:

  https://lkml.kernel.org/r/00000000000027204705a1354443@google.com

The reproducer does not set NLM_F_ACK in a dump request so that extack
is null and nl_set_extack_cookie_u32() tries to write at address 0x10.

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

Now I see that I was mistaken by the name and nl80211_pmsr_start() is in
fact ->doit() handler, not ->start(), so that it seems that it cannot be
really called with null info->extack. I'm not 100% sure of that either
(I would need to check the whole call path carefully again) but I'll
drop the second Fixes line.

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

From my point of view, having to keep in mind that NL_SET_ERR_MSG* are
no-op if extack is null but nl_set_extack_cookie_u{64,32} would crash
seems very inconvenient and even if I add the check into
ethnl_parse_header(), sooner or later someone is going to fall into the
same trap. Thus I believe that if there is a need for a warning when
nl80211_pmsr_start() is unexpectedly called with null info->extack, such
check should be done in nl80211_pmsr_start(), not by letting
nl_set_extack_cookie_u64() crash.

Michal