[PATCH net-next] net: ethtool: fix NULL pointer dereference in stats_prepare_data()

[PATCH net-next] net: ethtool: fix NULL pointer dereference in stats_prepare_data()

[Vladimir Oltean]

In the following call path:

ethnl_default_dumpit
-> ethnl_default_dump_one
   -> ctx->ops->prepare_data
      -> stats_prepare_data

struct genl_info *info will be passed as NULL, and stats_prepare_data()
dereferences it while getting the extended ack pointer.

To avoid that, just set the extack to NULL if "info" is NULL, since the
netlink extack handling messages know how to deal with that.

The pattern "info ? info->extack : NULL" is present in quite a few other
"prepare_data" implementations, so it's clear that it's a more general
problem to be dealt with at a higher level, but the code should have at
least adhered to the current conventions to avoid the NULL dereference.

Fixes: 04692c9020b7 ("net: ethtool: netlink: retrieve stats from multiple sources (eMAC, pMAC)")
Reported-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/ethtool/stats.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ethtool/stats.c b/net/ethtool/stats.c
index 7294be5855d4..010ed19ccc99 100644
--- a/net/ethtool/stats.c
+++ b/net/ethtool/stats.c
@@ -117,9 +117,9 @@ static int stats_prepare_data(const struct ethnl_req_info *req_base,
 			      struct genl_info *info)
 {
 	const struct stats_req_info *req_info = STATS_REQINFO(req_base);
+	struct netlink_ext_ack *extack = info ? info->extack : NULL;
 	struct stats_reply_data *data = STATS_REPDATA(reply_base);
 	enum ethtool_mac_stats_src src = req_info->src;
-	struct netlink_ext_ack *extack = info->extack;
 	struct net_device *dev = reply_base->dev;
 	int ret;
 
-- 
2.34.1

Re: [PATCH net-next] net: ethtool: fix NULL pointer dereference in stats_prepare_data()

[Leon Romanovsky]

On Tue, Jan 24, 2023 at 01:08:01PM +0200, Vladimir Oltean wrote:
> In the following call path:
> 
> ethnl_default_dumpit
> -> ethnl_default_dump_one
>    -> ctx->ops->prepare_data
>       -> stats_prepare_data
> 
> struct genl_info *info will be passed as NULL, and stats_prepare_data()
> dereferences it while getting the extended ack pointer.
> 
> To avoid that, just set the extack to NULL if "info" is NULL, since the
> netlink extack handling messages know how to deal with that.
> 
> The pattern "info ? info->extack : NULL" is present in quite a few other
> "prepare_data" implementations, so it's clear that it's a more general
> problem to be dealt with at a higher level, but the code should have at
> least adhered to the current conventions to avoid the NULL dereference.
> 
> Fixes: 04692c9020b7 ("net: ethtool: netlink: retrieve stats from multiple sources (eMAC, pMAC)")
> Reported-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/ethtool/stats.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH net-next] net: ethtool: fix NULL pointer dereference in stats_prepare_data()

[Jakub Kicinski]

On Tue, 24 Jan 2023 13:08:01 +0200 Vladimir Oltean wrote:
> In the following call path:
> 
> ethnl_default_dumpit
> -> ethnl_default_dump_one
>    -> ctx->ops->prepare_data
>       -> stats_prepare_data  
> 
> struct genl_info *info will be passed as NULL, and stats_prepare_data()
> dereferences it while getting the extended ack pointer.
> 
> To avoid that, just set the extack to NULL if "info" is NULL, since the
> netlink extack handling messages know how to deal with that.
> 
> The pattern "info ? info->extack : NULL" is present in quite a few other
> "prepare_data" implementations, so it's clear that it's a more general
> problem to be dealt with at a higher level, but the code should have at
> least adhered to the current conventions to avoid the NULL dereference.

Choose one:
 - you disagree with my comment on the report
 - you don't think that we should mix the immediate fix with the
   structural change
 - you agree but "don't have the time" to fix this properly

Re: [PATCH net-next] net: ethtool: fix NULL pointer dereference in stats_prepare_data()

[Vladimir Oltean]

On Tue, Jan 24, 2023 at 04:23:47PM -0800, Jakub Kicinski wrote:
> On Tue, 24 Jan 2023 13:08:01 +0200 Vladimir Oltean wrote:
> > In the following call path:
> > 
> > ethnl_default_dumpit
> > -> ethnl_default_dump_one
> >    -> ctx->ops->prepare_data
> >       -> stats_prepare_data  
> > 
> > struct genl_info *info will be passed as NULL, and stats_prepare_data()
> > dereferences it while getting the extended ack pointer.
> > 
> > To avoid that, just set the extack to NULL if "info" is NULL, since the
> > netlink extack handling messages know how to deal with that.
> > 
> > The pattern "info ? info->extack : NULL" is present in quite a few other
> > "prepare_data" implementations, so it's clear that it's a more general
> > problem to be dealt with at a higher level, but the code should have at
> > least adhered to the current conventions to avoid the NULL dereference.
> 
> Choose one:
>  - you disagree with my comment on the report
>  - you don't think that we should mix the immediate fix with the
>    structural change
>  - you agree but "don't have the time" to fix this properly

Yeah, sorry, I shouldn't have left your question unanswered ("should we make
a fake struct genl_info * to pass here?") - but I don't think I'm qualified
enough to have an opinion, either. Whereas the immediate fix is neutral
enough to not be controversial, or so I thought.

The problem is not so much "the time to fix this properly", but rather,
I'm not even sure how to trigger the ethtool dumpit() code...

Re: [PATCH net-next] net: ethtool: fix NULL pointer dereference in stats_prepare_data()

[Jakub Kicinski]

On Wed, 25 Jan 2023 02:45:17 +0200 Vladimir Oltean wrote:
> > Choose one:
> >  - you disagree with my comment on the report
> >  - you don't think that we should mix the immediate fix with the
> >    structural change
> >  - you agree but "don't have the time" to fix this properly  
> 
> Yeah, sorry, I shouldn't have left your question unanswered ("should we make
> a fake struct genl_info * to pass here?") - but I don't think I'm qualified
> enough to have an opinion, either. Whereas the immediate fix is neutral
> enough to not be controversial, or so I thought.
> 
> The problem is not so much "the time to fix this properly", but rather,
> I'm not even sure how to trigger the ethtool dumpit() code...

Ack, makes sense. I just wanted to make sure you weren't disagreeing.

Re: [PATCH net-next] net: ethtool: fix NULL pointer dereference in stats_prepare_data()

[Vladimir Oltean]

On Tue, Jan 24, 2023 at 04:52:23PM -0800, Jakub Kicinski wrote:
> On Wed, 25 Jan 2023 02:45:17 +0200 Vladimir Oltean wrote:
> > > Choose one:
> > >  - you disagree with my comment on the report
> > >  - you don't think that we should mix the immediate fix with the
> > >    structural change
> > >  - you agree but "don't have the time" to fix this properly  
> > 
> > Yeah, sorry, I shouldn't have left your question unanswered ("should we make
> > a fake struct genl_info * to pass here?") - but I don't think I'm qualified
> > enough to have an opinion, either. Whereas the immediate fix is neutral
> > enough to not be controversial, or so I thought.
> > 
> > The problem is not so much "the time to fix this properly", but rather,
> > I'm not even sure how to trigger the ethtool dumpit() code...
> 
> Ack, makes sense. I just wanted to make sure you weren't disagreeing.

Since we're talking about it, do you know how to exercise that code?

Re: [PATCH net-next] net: ethtool: fix NULL pointer dereference in stats_prepare_data()

[Jakub Kicinski]

On Wed, 25 Jan 2023 02:53:33 +0200 Vladimir Oltean wrote:
> > > Yeah, sorry, I shouldn't have left your question unanswered ("should we make
> > > a fake struct genl_info * to pass here?") - but I don't think I'm qualified
> > > enough to have an opinion, either. Whereas the immediate fix is neutral
> > > enough to not be controversial, or so I thought.
> > > 
> > > The problem is not so much "the time to fix this properly", but rather,
> > > I'm not even sure how to trigger the ethtool dumpit() code...  
> > 
> > Ack, makes sense. I just wanted to make sure you weren't disagreeing.  
> 
> Since we're talking about it, do you know how to exercise that code?

Your MM code? Not really. But the code is fairly copy'n'paste.
I test what netdevsim supports (netdevsim support for MM highly
encouraged).

Re: [PATCH net-next] net: ethtool: fix NULL pointer dereference in stats_prepare_data()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Tue, 24 Jan 2023 13:08:01 +0200 you wrote:
> In the following call path:
> 
> ethnl_default_dumpit
> -> ethnl_default_dump_one
>    -> ctx->ops->prepare_data
>       -> stats_prepare_data
> 
> [...]

Here is the summary with links:
  - [net-next] net: ethtool: fix NULL pointer dereference in stats_prepare_data()
    https://git.kernel.org/netdev/net-next/c/c96de136329b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html