Messages in this thread | | | Date | Fri, 20 Mar 2020 23:27:35 +0100 | From | Michal Kubecek <> | Subject | Re: [PATCH net] netlink: check for null extack in cookie helpers |
| |
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
| |