| Message ID | 20230124110801.3628545-1-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c96de136329b38172f21214021fc30d67f05c399 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: ethtool: fix NULL pointer dereference in stats_prepare_data() | expand |
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>
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
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...
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.
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?
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).
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!
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;
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(-)