| Message ID | 20210316132416.83578-1-galpress@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [for-next] RDMA/efa: Use strscpy instead of strlcpy | expand |
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
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/
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
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.
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
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
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);