Messages in this thread | | | Date | Thu, 21 Nov 2019 15:07:33 +0300 | From | Dan Carpenter <> | Subject | Re: [Cocci] [PATCH] net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol() |
| |
On Thu, Nov 21, 2019 at 12:19:17PM +0100, Michal Kubecek wrote: > On Thu, Nov 21, 2019 at 11:23:34AM +0100, Enrico Weigelt, metux IT consult wrote: > > On 26.10.19 21:40, Joe Perches wrote: > > > On Sat, 2019-10-26 at 15:54 +0800, zhanglin wrote: > > >> memset() the structure ethtool_wolinfo that has padded bytes > > >> but the padded bytes have not been zeroed out. > > > [] > > >> diff --git a/net/core/ethtool.c b/net/core/ethtool.c > > > [] > > >> @@ -1471,11 +1471,13 @@ static int ethtool_reset(struct net_device *dev, char __user *useraddr) > > >> > > >> static int ethtool_get_wol(struct net_device *dev, char __user *useraddr) > > >> { > > >> - struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL }; > > >> + struct ethtool_wolinfo wol; > > >> > > >> if (!dev->ethtool_ops->get_wol) > > >> return -EOPNOTSUPP; > > >> > > >> + memset(&wol, 0, sizeof(struct ethtool_wolinfo)); > > >> + wol.cmd = ETHTOOL_GWOL; > > >> dev->ethtool_ops->get_wol(dev, &wol); > > >> > > >> if (copy_to_user(useraddr, &wol, sizeof(wol))) > > > > > > It seems likely there are more of these. > > > > > > Is there any way for coccinelle to find them? > > > > Just curios: is static struct initialization (on stack) something that > > should be avoided ? I've been under the impression that static > > initialization allows thinner code and gives the compiler better chance > > for optimizations. > > Not in general. The (potential) problem here is that the structure has > padding and it is as a whole (i.e. including the padding) copied to > userspace. While I'm not aware of a compiler that wouldn't actually > initialize the whole data block including the padding in this case, the > C standard provides no guarantee about that so that to be sure we cannot > leak leftover kernel data to userspace, we need to explicitly initialize > the whole block.
GCC will not always initialize the struct holes. This patch fixes a real bug that GCC on my system (v7.4)
regards, dan carpenter
| |