Re: [PATCH] net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol()

On Sat, 2019-10-26 at 17:24 +0300, Dan Carpenter wrote:

On Sat, Oct 26, 2019 at 03:54:16PM +0800, zhanglin wrote:

quoted

memset() the structure ethtool_wolinfo that has padded bytes
but the padded bytes have not been zeroed out.

[]

quoted

diff --git a/net/core/ethtool.c b/net/core/ethtool.c

[]

quoted

@@ -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_wolinf wol = { .cmd = ETHTOOL_GWOL };
+	struct ethtool_wolinfo wol;
 

How did you detect that they weren't initialized?  Is this a KASAN
thing?

Most of the time GCC will zero out the padding bytes when you have an
initializer like this, but sometimes it just makes the intialization a
series of assignments which leaves the holes uninitialized.  I wish I
knew the rules so that I could check for it in Smatch.  Or even better,
I wish that there were an option to always zero the holes in this
situation...

The standard doesn't specify what happens to the padding so
it's not just for gcc, it's compiler dependent.

So anything that's used in a copy_to_user with any possible
padding should either be zalloc'd or memset before assigned.

In this case:

include/uapi/linux/ethtool.h:#define SOPASS_MAX 6

and

include/uapi/linux/ethtool.h:struct ethtool_wolinfo {
include/uapi/linux/ethtool.h-   __u32   cmd;
include/uapi/linux/ethtool.h-   __u32   supported;
include/uapi/linux/ethtool.h-   __u32   wolopts;
include/uapi/linux/ethtool.h-   __u8    sopass[SOPASS_MAX];
include/uapi/linux/ethtool.h-};

so there's likely a couple bytes of trailing padding.