[PATCH for-next] RDMA/efa: Use strscpy instead of strlcpy

[PATCH for-next] RDMA/efa: Use strscpy instead of strlcpy

[Gal Pressman]

The strlcpy function doesn't limit the source length, use the preferred
strscpy function instead.

Reviewed-by: Firas JahJah <firasj@amazon.com>
Reviewed-by: Yossi Leybovich <sleybo@amazon.com>
Signed-off-by: Gal Pressman <galpress@amazon.com>
---
 drivers/infiniband/hw/efa/efa_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/efa/efa_main.c b/drivers/infiniband/hw/efa/efa_main.c
index 0f578734bddb..a8da96ef2be8 100644
--- a/drivers/infiniband/hw/efa/efa_main.c
+++ b/drivers/infiniband/hw/efa/efa_main.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
 /*
- * Copyright 2018-2020 Amazon.com, Inc. or its affiliates. All rights reserved.
+ * Copyright 2018-2021 Amazon.com, Inc. or its affiliates. All rights reserved.
  */
 
 #include <linux/module.h>
@@ -209,10 +209,10 @@ static void efa_set_host_info(struct efa_dev *dev)
 	if (!hinf)
 		return;
 
-	strlcpy(hinf->os_dist_str, utsname()->release,
+	strscpy(hinf->os_dist_str, utsname()->release,
 		min(sizeof(hinf->os_dist_str), sizeof(utsname()->release)));
 	hinf->os_type = EFA_ADMIN_OS_LINUX;
-	strlcpy(hinf->kernel_ver_str, utsname()->version,
+	strscpy(hinf->kernel_ver_str, utsname()->version,
 		min(sizeof(hinf->kernel_ver_str), sizeof(utsname()->version)));
 	hinf->kernel_ver = LINUX_VERSION_CODE;
 	EFA_SET(&hinf->driver_ver, EFA_ADMIN_HOST_INFO_DRIVER_MAJOR, 0);
-- 
2.31.0

Re: [PATCH for-next] RDMA/efa: Use strscpy instead of strlcpy

[Jason Gunthorpe]

On Tue, Mar 16, 2021 at 03:24:16PM +0200, Gal Pressman wrote:
> The strlcpy function doesn't limit the source length, use the preferred
> strscpy function instead.

Why do we need to limit the source length here? Either this is a bug
because the source string is no NULL terminated or it is OK as is?

Jason

Re: [PATCH for-next] RDMA/efa: Use strscpy instead of strlcpy

[Gal Pressman]

On 22/03/2021 15:01, Jason Gunthorpe wrote:
> On Tue, Mar 16, 2021 at 03:24:16PM +0200, Gal Pressman wrote:
>> The strlcpy function doesn't limit the source length, use the preferred
>> strscpy function instead.
> 
> Why do we need to limit the source length here? Either this is a bug
> because the source string is no NULL terminated or it is OK as is?

It's not a bug as is, but it addresses checkpatch's warning:
WARNING: Prefer strscpy over strlcpy - see: https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/

Re: [PATCH for-next] RDMA/efa: Use strscpy instead of strlcpy

[Jason Gunthorpe]

On Mon, Mar 22, 2021 at 03:11:33PM +0200, Gal Pressman wrote:
> 
> On 22/03/2021 15:01, Jason Gunthorpe wrote:
> > On Tue, Mar 16, 2021 at 03:24:16PM +0200, Gal Pressman wrote:
> >> The strlcpy function doesn't limit the source length, use the preferred
> >> strscpy function instead.
> > 
> > Why do we need to limit the source length here? Either this is a bug
> > because the source string is no NULL terminated or it is OK as is?
> 
> It's not a bug as is, but it addresses checkpatch's warning:
> WARNING: Prefer strscpy over strlcpy - see: https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/

Okay.. but why is it so weird:

        strscpy(hinf->kernel_ver_str, utsname()->version,
                min(sizeof(hinf->kernel_ver_str), sizeof(utsname()->version)));
 
?

utsname()->version is null terminated, yes? Why does it need to be
min'd?

Jason

Re: [PATCH for-next] RDMA/efa: Use strscpy instead of strlcpy

[Gal Pressman]

On 22/03/2021 18:55, Jason Gunthorpe wrote:
> On Mon, Mar 22, 2021 at 03:11:33PM +0200, Gal Pressman wrote:
>>
>> On 22/03/2021 15:01, Jason Gunthorpe wrote:
>>> On Tue, Mar 16, 2021 at 03:24:16PM +0200, Gal Pressman wrote:
>>>> The strlcpy function doesn't limit the source length, use the preferred
>>>> strscpy function instead.
>>>
>>> Why do we need to limit the source length here? Either this is a bug
>>> because the source string is no NULL terminated or it is OK as is?
>>
>> It's not a bug as is, but it addresses checkpatch's warning:
>> WARNING: Prefer strscpy over strlcpy - see: https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/
> 
> Okay.. but why is it so weird:
> 
>         strscpy(hinf->kernel_ver_str, utsname()->version,
>                 min(sizeof(hinf->kernel_ver_str), sizeof(utsname()->version)));
> 
> ?
> 
> utsname()->version is null terminated, yes? Why does it need to be
> min'd?

The size of the kernel buffer is different than the device buffer (65B vs 32B),
the min() is there to prevent overflow regardless of the NULL termination.
A NULL terminated 60 bytes utsname would be truncated to 32 bytes.

Re: [PATCH for-next] RDMA/efa: Use strscpy instead of strlcpy

[Jason Gunthorpe]

On Mon, Mar 22, 2021 at 07:14:35PM +0200, Gal Pressman wrote:
> On 22/03/2021 18:55, Jason Gunthorpe wrote:
> > On Mon, Mar 22, 2021 at 03:11:33PM +0200, Gal Pressman wrote:
> >>
> >> On 22/03/2021 15:01, Jason Gunthorpe wrote:
> >>> On Tue, Mar 16, 2021 at 03:24:16PM +0200, Gal Pressman wrote:
> >>>> The strlcpy function doesn't limit the source length, use the preferred
> >>>> strscpy function instead.
> >>>
> >>> Why do we need to limit the source length here? Either this is a bug
> >>> because the source string is no NULL terminated or it is OK as is?
> >>
> >> It's not a bug as is, but it addresses checkpatch's warning:
> >> WARNING: Prefer strscpy over strlcpy - see: https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/
> > 
> > Okay.. but why is it so weird:
> > 
> >         strscpy(hinf->kernel_ver_str, utsname()->version,
> >                 min(sizeof(hinf->kernel_ver_str), sizeof(utsname()->version)));
> > 
> > ?
> > 
> > utsname()->version is null terminated, yes? Why does it need to be
> > min'd?
> 
> The size of the kernel buffer is different than the device buffer (65B vs 32B),
> the min() is there to prevent overflow regardless of the NULL termination.
> A NULL terminated 60 bytes utsname would be truncated to 32 bytes.

I don't understand.

If version is NULL terminated than this:

   strscpy(hinf->kernel_ver_str, utsname()->version,
           sizeof(hinf->kernel_ver_str))

Is the only thing needed? The whole point of strscpy is that it
truncates the string to fit the output.

Jason

Re: [PATCH for-next] RDMA/efa: Use strscpy instead of strlcpy

[Gal Pressman]

On 23/03/2021 2:21, Jason Gunthorpe wrote:
> On Mon, Mar 22, 2021 at 07:14:35PM +0200, Gal Pressman wrote:
>> On 22/03/2021 18:55, Jason Gunthorpe wrote:
>>> On Mon, Mar 22, 2021 at 03:11:33PM +0200, Gal Pressman wrote:
>>>>
>>>> On 22/03/2021 15:01, Jason Gunthorpe wrote:
>>>>> On Tue, Mar 16, 2021 at 03:24:16PM +0200, Gal Pressman wrote:
>>>>>> The strlcpy function doesn't limit the source length, use the preferred
>>>>>> strscpy function instead.
>>>>>
>>>>> Why do we need to limit the source length here? Either this is a bug
>>>>> because the source string is no NULL terminated or it is OK as is?
>>>>
>>>> It's not a bug as is, but it addresses checkpatch's warning:
>>>> WARNING: Prefer strscpy over strlcpy - see: https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/
>>>
>>> Okay.. but why is it so weird:
>>>
>>>         strscpy(hinf->kernel_ver_str, utsname()->version,
>>>                 min(sizeof(hinf->kernel_ver_str), sizeof(utsname()->version)));
>>>
>>> ?
>>>
>>> utsname()->version is null terminated, yes? Why does it need to be
>>> min'd?
>>
>> The size of the kernel buffer is different than the device buffer (65B vs 32B),
>> the min() is there to prevent overflow regardless of the NULL termination.
>> A NULL terminated 60 bytes utsname would be truncated to 32 bytes.
> 
> I don't understand.
> 
> If version is NULL terminated than this:
> 
>    strscpy(hinf->kernel_ver_str, utsname()->version,
>            sizeof(hinf->kernel_ver_str))
> 
> Is the only thing needed? The whole point of strscpy is that it
> truncates the string to fit the output.

You're right, will update the patch.
Thanks Jason