netdev - Re: [PATCH net] mlx4: Fix information leak on failure to read module EEPROM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <40aaf07aa7463c0fc6ca89aab36c622bfb789ba4.camel@mellanox.com>
Date:   Mon, 18 May 2020 16:47:05 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "ben@...adent.org.uk" <ben@...adent.org.uk>,
        Tariq Toukan <tariqt@...lanox.com>
CC:     "960702@...s.debian.org" <960702@...s.debian.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net] mlx4: Fix information leak on failure to read module
 EEPROM

On Sun, 2020-05-17 at 18:20 +0100, Ben Hutchings wrote:
> mlx4_en_get_module_eeprom() returns 0 even if it fails.  This results
> in copying an uninitialised (or partly initialised) buffer back to
> user-space.
> 
> Change it so that:
> 
> * In the special case that the DOM turns out not to be readable, the
>   remaining part of the buffer is cleared.  This should avoid a
>   regression when reading modules with this problem.
> 
> * In other error cases, the error code is propagated.
> 
> Reported-by: Yannis Aribaud <bugs@...ell.net>
> References: https://bugs.debian.org/960702
> Fixes: 7202da8b7f71 ("ethtool, net/mlx4_en: Cable info,
> get_module_info/...")
> Signed-off-by: Ben Hutchings <ben@...adent.org.uk>
> ---
> This is compile-tested only.  It should go to stable, if it is a
> correct fix.
> 
> Ben.
> 
>  drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> index 8a5ea2543670..6edc3177af1c 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
> @@ -2078,14 +2078,17 @@ static int mlx4_en_get_module_eeprom(struct
> net_device *dev,
>  		ret = mlx4_get_module_info(mdev->dev, priv->port,
>  					   offset, ee->len - i, data +
> i);
>  

I am not sure i see the issue in here, and why we need the partial
memset ?
first thing in this function we do:
memset(data, 0, ee->len);

and then mlx4_get_module_info() will only copy valid data only on
success.


> -		if (!ret) /* Done reading */
> +		if (!ret) {
> +			/* DOM was not readable after all */

actually if mlx4_get_module_info()  returns any non-negative value it
means how much data was read, so if it returns 0, it means that this
was the last iteration and we are done reading the eeprom.. 

so i would remove the above comment and the memset below is redundant
since we already memset the whole buffer before the while loop.

> +			memset(data + i, 0, ee->len - i);
>  			return 0;
> +		}
>  
>  		if (ret < 0) {
>  			en_err(priv,
>  			       "mlx4_get_module_info i(%d) offset(%d)
> bytes_to_read(%d) - FAILED (0x%x)\n",
>  			       i, offset, ee->len - i, ret);
> -			return 0;
> +			return ret;

I think returning error in here was the actual solution for your
problem. you can verify by looking in the kernel log and verify you see
the log message.

>  		}
>  
>  		i += ret;