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

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

[Michal Kubecek]

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

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")
Reported-by: syzbot+258a9089477493cea67b@syzkaller.appspotmail.com
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 include/linux/netlink.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 4090524c3462..60739d0cbf93 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -115,6 +115,8 @@ static inline void nl_set_extack_cookie_u64(struct netlink_ext_ack *extack,
 {
 	u64 __cookie = cookie;
 
+	if (!extack)
+		return;
 	memcpy(extack->cookie, &__cookie, sizeof(__cookie));
 	extack->cookie_len = sizeof(__cookie);
 }
@@ -124,6 +126,8 @@ static inline void nl_set_extack_cookie_u32(struct netlink_ext_ack *extack,
 {
 	u32 __cookie = cookie;
 
+	if (!extack)
+		return;
 	memcpy(extack->cookie, &__cookie, sizeof(__cookie));
 	extack->cookie_len = sizeof(__cookie);
 }
-- 
2.25.1

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

[Johannes Berg]

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

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

[Michal Kubecek]

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