* [PATCH net] mlx4: Fix information leak on failure to read module EEPROM
@ 2020-05-17 17:20 Ben Hutchings
2020-05-18 16:47 ` Saeed Mahameed
0 siblings, 1 reply; 3+ messages in thread
From: Ben Hutchings @ 2020-05-17 17:20 UTC (permalink / raw)
To: Tariq Toukan; +Cc: 960702, netdev
[-- Attachment #1: Type: text/plain, Size: 1680 bytes --]
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@d6bell.net>
References: https://bugs.debian.org/960702
Fixes: 7202da8b7f71 ("ethtool, net/mlx4_en: Cable info, get_module_info/...")
Signed-off-by: Ben Hutchings <ben@decadent.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);
- if (!ret) /* Done reading */
+ if (!ret) {
+ /* DOM was not readable after all */
+ 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 += ret;
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net] mlx4: Fix information leak on failure to read module EEPROM
2020-05-17 17:20 [PATCH net] mlx4: Fix information leak on failure to read module EEPROM Ben Hutchings
@ 2020-05-18 16:47 ` Saeed Mahameed
2020-05-18 18:16 ` Ben Hutchings
0 siblings, 1 reply; 3+ messages in thread
From: Saeed Mahameed @ 2020-05-18 16:47 UTC (permalink / raw)
To: ben, Tariq Toukan; +Cc: 960702, netdev
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@d6bell.net>
> References: https://bugs.debian.org/960702
> Fixes: 7202da8b7f71 ("ethtool, net/mlx4_en: Cable info,
> get_module_info/...")
> Signed-off-by: Ben Hutchings <ben@decadent.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;
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net] mlx4: Fix information leak on failure to read module EEPROM
2020-05-18 16:47 ` Saeed Mahameed
@ 2020-05-18 18:16 ` Ben Hutchings
0 siblings, 0 replies; 3+ messages in thread
From: Ben Hutchings @ 2020-05-18 18:16 UTC (permalink / raw)
To: Saeed Mahameed, Tariq Toukan; +Cc: 960702, netdev
[-- Attachment #1: Type: text/plain, Size: 2355 bytes --]
On Mon, 2020-05-18 at 16:47 +0000, Saeed Mahameed wrote:
> 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.
[...]
> 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.
Wow, sorry, I don't know how I missed that. So this is not the bug I
was looking for.
>
> > - 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.
Right.
> > + 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.
The original bug report (https://bugs.debian.org/960702) says that
ethtool reports different values depending on whether its output is
redirected. Although returning all-zeroes for the unreadable part
might be wrong, it doesn't explain that behaviour.
Perhaps if the timing of the I²C reads is marginal, varying numbers of
bytes of DOM information might be readable? But I don't see how
redirection of ethtool's output would affect that. It uses a single
ioctl to read everything, and the kernel controls timing within that.
So I am mystified about what is going on here. Maybe there is a bug in
ethtool, but I'm not seeing it.
Ben.
> > }
> >
> > i += ret;
--
Ben Hutchings
The two most common things in the universe are hydrogen and stupidity.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-05-18 18:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-17 17:20 [PATCH net] mlx4: Fix information leak on failure to read module EEPROM Ben Hutchings
2020-05-18 16:47 ` Saeed Mahameed
2020-05-18 18:16 ` Ben Hutchings
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).