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

[Johannes Berg <johannes@sipsolutions.net>]

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