diff mbox series

mlx4: use snprintf() instead of sprintf() for safety

Message ID 20221122130453.730657-1-pkosyh@yandex.ru
State New
Headers
Series mlx4: use snprintf() instead of sprintf() for safety |

Commit Message

Peter Kosyh Nov. 22, 2022, 1:04 p.m. UTC 
  Use snprintf() to avoid the potential buffer overflow. Although in the
current code this is hardly possible, the safety is unclean.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Peter Kosyh <pkosyh@yandex.ru>
---
 drivers/net/ethernet/mellanox/mlx4/main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Leon Romanovsky Nov. 22, 2022, 2:48 p.m. UTC | #1 
On Tue, Nov 22, 2022 at 04:04:53PM +0300, Peter Kosyh wrote:
> Use snprintf() to avoid the potential buffer overflow. Although in the
> current code this is hardly possible, the safety is unclean.

Let's fix the tools instead. The kernel code is correct.

Thanks

> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Peter Kosyh <pkosyh@yandex.ru>
> ---
>  drivers/net/ethernet/mellanox/mlx4/main.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
> index d3fc86cd3c1d..0616d352451b 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/main.c
> @@ -3057,7 +3057,8 @@ static int mlx4_init_port_info(struct mlx4_dev *dev, int port)
>  		info->base_qpn = mlx4_get_base_qpn(dev, port);
>  	}
>  
> -	sprintf(info->dev_name, "mlx4_port%d", port);
> +	snprintf(info->dev_name, sizeof(info->dev_name),
> +		 "mlx4_port%d", port);
>  	info->port_attr.attr.name = info->dev_name;
>  	if (mlx4_is_mfunc(dev)) {
>  		info->port_attr.attr.mode = 0444;
> @@ -3077,7 +3078,8 @@ static int mlx4_init_port_info(struct mlx4_dev *dev, int port)
>  		return err;
>  	}
>  
> -	sprintf(info->dev_mtu_name, "mlx4_port%d_mtu", port);
> +	snprintf(info->dev_mtu_name, sizeof(info->dev_mtu_name),
> +		 "mlx4_port%d_mtu", port);
>  	info->port_mtu_attr.attr.name = info->dev_mtu_name;
>  	if (mlx4_is_mfunc(dev)) {
>  		info->port_mtu_attr.attr.mode = 0444;
> -- 
> 2.38.1
>
Jakub Kicinski Nov. 22, 2022, 8:12 p.m. UTC | #2 
On Tue, 22 Nov 2022 16:48:15 +0200 Leon Romanovsky wrote:
> On Tue, Nov 22, 2022 at 04:04:53PM +0300, Peter Kosyh wrote:
> > Use snprintf() to avoid the potential buffer overflow. Although in the
> > current code this is hardly possible, the safety is unclean.  
> 
> Let's fix the tools instead. The kernel code is correct.

I'm guessing the code is correct because port can't be a high value?
Otherwise, if I'm counting right, large enough port representation
(e.g. 99999999) could overflow the string. If that's the case - how
would they "fix the tool" to know the port is always a single digit?
Saeed Mahameed Nov. 23, 2022, 4:43 a.m. UTC | #3 
On 22 Nov 12:12, Jakub Kicinski wrote:
>On Tue, 22 Nov 2022 16:48:15 +0200 Leon Romanovsky wrote:
>> On Tue, Nov 22, 2022 at 04:04:53PM +0300, Peter Kosyh wrote:
>> > Use snprintf() to avoid the potential buffer overflow. Although in the
>> > current code this is hardly possible, the safety is unclean.
>>
>> Let's fix the tools instead. The kernel code is correct.
>
>I'm guessing the code is correct because port can't be a high value?
>Otherwise, if I'm counting right, large enough port representation
>(e.g. 99999999) could overflow the string. If that's the case - how
>would they "fix the tool" to know the port is always a single digit?

+1 

FWIW,

Reviewed-by: Saeed Mahameed <saeed@kernel.org>
Leon Romanovsky Nov. 23, 2022, 6:40 a.m. UTC | #4 
On Tue, Nov 22, 2022 at 12:12:23PM -0800, Jakub Kicinski wrote:
> On Tue, 22 Nov 2022 16:48:15 +0200 Leon Romanovsky wrote:
> > On Tue, Nov 22, 2022 at 04:04:53PM +0300, Peter Kosyh wrote:
> > > Use snprintf() to avoid the potential buffer overflow. Although in the
> > > current code this is hardly possible, the safety is unclean.  
> > 
> > Let's fix the tools instead. The kernel code is correct.
> 
> I'm guessing the code is correct because port can't be a high value?

Yes, port value is provided as input to mlx4_init_port_info() and it is
capped by MLX4_MAX_PORTS, which is 2.

> Otherwise, if I'm counting right, large enough port representation
> (e.g. 99999999) could overflow the string. If that's the case - how
> would they "fix the tool" to know the port is always a single digit?

I may admit that I don't know how hard or easy to implement it, but it
will be great if tool would be able to understand that dev->caps.num_ports
are not really dynamic values, but constant ones.

However, I don't mind if we merge it.

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c
index d3fc86cd3c1d..0616d352451b 100644
--- a/drivers/net/ethernet/mellanox/mlx4/main.c
+++ b/drivers/net/ethernet/mellanox/mlx4/main.c
@@ -3057,7 +3057,8 @@  static int mlx4_init_port_info(struct mlx4_dev *dev, int port)
 		info->base_qpn = mlx4_get_base_qpn(dev, port);
 	}
 
-	sprintf(info->dev_name, "mlx4_port%d", port);
+	snprintf(info->dev_name, sizeof(info->dev_name),
+		 "mlx4_port%d", port);
 	info->port_attr.attr.name = info->dev_name;
 	if (mlx4_is_mfunc(dev)) {
 		info->port_attr.attr.mode = 0444;
@@ -3077,7 +3078,8 @@  static int mlx4_init_port_info(struct mlx4_dev *dev, int port)
 		return err;
 	}
 
-	sprintf(info->dev_mtu_name, "mlx4_port%d_mtu", port);
+	snprintf(info->dev_mtu_name, sizeof(info->dev_mtu_name),
+		 "mlx4_port%d_mtu", port);
 	info->port_mtu_attr.attr.name = info->dev_mtu_name;
 	if (mlx4_is_mfunc(dev)) {
 		info->port_mtu_attr.attr.mode = 0444;