| Message ID | 20221122130453.730657-1-pkosyh@yandex.ru (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | mlx4: use snprintf() instead of sprintf() for safety | expand |
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 >
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?
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>
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 --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;
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(-)