[1/1] net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol()

From: Avijit Kanti Das <avijitnsec@codeaurora.org>

CVE-2014-9900

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

Change-Id: If3fd2d872a1b1ab9521d937b86a29fc468a8bbfe
Signed-off-by: Avijit Kanti Das <avijitnsec@codeaurora.org>
Signed-off-by: Brad Figg <brad.figg@canonical.com>
---
 net/core/ethtool.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

On Thu, Jun 08, 2017 at 06:41:15AM -0700, Brad Figg wrote:
> From: Avijit Kanti Das <avijitnsec@codeaurora.org>
> 
> CVE-2014-9900
> 
> memset() the structure ethtool_wolinfo that has padded bytes
> but the padded bytes have not been zeroed out.
> 
> Change-Id: If3fd2d872a1b1ab9521d937b86a29fc468a8bbfe
> Signed-off-by: Avijit Kanti Das <avijitnsec@codeaurora.org>
> Signed-off-by: Brad Figg <brad.figg@canonical.com>
> ---
>  net/core/ethtool.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 23b3394..8fc6595 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1160,11 +1160,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)))

I'm not totally convinced there is any bug here. gcc seems to promise
that omitted fields of a designated initialized will be set to 0 [1].
Nothing is said there about padding bytes, not that it looks like there
should be any in that structure.

At any rate, I suppose the patch does no harm and leaves us with no
possible ambiguity about the data which is copied to userspace, so:

Acked-by: Seth Forshee <seth.forshee@canonical.com>

However, given that it's not an upstream patch shouldn't it be SAUCE?

[1] https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html

On 08/06/17 14:41, Brad Figg wrote:
> From: Avijit Kanti Das <avijitnsec@codeaurora.org>
> 
> CVE-2014-9900
> 
> memset() the structure ethtool_wolinfo that has padded bytes
> but the padded bytes have not been zeroed out.
> 
> Change-Id: If3fd2d872a1b1ab9521d937b86a29fc468a8bbfe
> Signed-off-by: Avijit Kanti Das <avijitnsec@codeaurora.org>
> Signed-off-by: Brad Figg <brad.figg@canonical.com>
> ---
>  net/core/ethtool.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 23b3394..8fc6595 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1160,11 +1160,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)))
> 
While I concur with Seth's view on this patch, I deem it useful to
explicitly ensure the empty padding in the data structure is zero'd to
stop information leaks.

Acked-by: Colin Ian King <colin.king@canonical.com>

On Thu, Jun 08, 2017 at 06:41:15AM -0700, Brad Figg wrote:
> From: Avijit Kanti Das <avijitnsec@codeaurora.org>
> 
> CVE-2014-9900
> 
> memset() the structure ethtool_wolinfo that has padded bytes
> but the padded bytes have not been zeroed out.
> 
> Change-Id: If3fd2d872a1b1ab9521d937b86a29fc468a8bbfe
> Signed-off-by: Avijit Kanti Das <avijitnsec@codeaurora.org>
> Signed-off-by: Brad Figg <brad.figg@canonical.com>

Applied to artful/master-next and unstable/master with the subject
"UBUNTU: SAUCE: (no-up) net: Zeroing the structure ethtool_wolinfo in
ethtool_get_wol()."

Applied to T/X/Y/Z master-next.

Thanks,
-Stefan