Messages in this thread Patch in this message | | | Subject | Re: [Linuxarm] [PATCH v1] drm/nouveau/device: append a NUL-terminated character for the string which filled by strncpy() | From | luojiaxing <> | Date | Sat, 27 Feb 2021 17:20:24 +0800 |
| |
On 2021/2/26 9:01, Song Bao Hua (Barry Song) wrote: > >> -----Original Message----- >> From: Luo Jiaxing [mailto:luojiaxing@huawei.com] >> Sent: Friday, February 26, 2021 12:39 AM >> To: nouveau@lists.freedesktop.org; dri-devel@lists.freedesktop.org; >> bskeggs@redhat.com >> Cc: linux-kernel@vger.kernel.org; linuxarm@openeuler.org; luojiaxing >> <luojiaxing@huawei.com> >> Subject: [Linuxarm] [PATCH v1] drm/nouveau/device: append a NUL-terminated >> character for the string which filled by strncpy() >> >> Following warning is found when using W=1 to build kernel: >> >> In function ‘nvkm_udevice_info’, >> inlined from ‘nvkm_udevice_mthd’ at >> drivers/gpu/drm/nouveau/nvkm/engine/device/user.c:195:10: >> drivers/gpu/drm/nouveau/nvkm/engine/device/user.c:164:2: warning: ‘strncpy’ >> specified bound 16 equals destination size [-Wstringop-truncation] >> 164 | strncpy(args->v0.chip, device->chip->name, sizeof(args->v0.chip)); >> drivers/gpu/drm/nouveau/nvkm/engine/device/user.c:165:2: warning: ‘strncpy’ >> specified bound 64 equals destination size [-Wstringop-truncation] >> 165 | strncpy(args->v0.name, device->name, sizeof(args->v0.name)); >> >> The reason of this warning is strncpy() does not guarantee that the >> destination buffer will be NUL terminated. If the length of source string >> is bigger than number we set by third input parameter, only first [number] >> of characters is copied to the destination, and no NUL-terminated is >> automatically added. There are some potential risks. >> >> Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com> >> --- >> drivers/gpu/drm/nouveau/nvkm/engine/device/user.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c >> b/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c >> index fea9d8f..2a32fe0 100644 >> --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c >> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c >> @@ -161,8 +161,10 @@ nvkm_udevice_info(struct nvkm_udevice *udev, void *data, >> u32 size) >> if (imem && args->v0.ram_size > 0) >> args->v0.ram_user = args->v0.ram_user - imem->reserved; >> >> - strncpy(args->v0.chip, device->chip->name, sizeof(args->v0.chip)); >> - strncpy(args->v0.name, device->name, sizeof(args->v0.name)); >> + strncpy(args->v0.chip, device->chip->name, sizeof(args->v0.chip) - 1); >> + args->v0.chip[sizeof(args->v0.chip) - 1] = '\0'; >> + strncpy(args->v0.name, device->name, sizeof(args->v0.name) - 1); >> + args->v0.name[sizeof(args->v0.name) - 1] = '\0'; > > Isn't it better to use snprintf()?
yes, you are right, snprintf() is better. Most of drivers use snprintf() to format a string,
but still some examples in kernel that use it for copy.
I modify to code to the follow and I think it's the same with strncpy but more safety
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c index fea9d8f..4bf65bb 100644 --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c @@ -161,8 +161,8 @@ nvkm_udevice_info(struct nvkm_udevice *udev, void *data, u32 size) if (imem && args->v0.ram_size > 0) args->v0.ram_user = args->v0.ram_user - imem->reserved;
- strncpy(args->v0.chip, device->chip->name, sizeof(args->v0.chip)); - strncpy(args->v0.name, device->name, sizeof(args->v0.name)); + snprintf(args->v0.chip, sizeof(args->v0.chip), "%s", device->chip->name); + snprintf(args->v0.name, sizeof(args->v0.name), "%s", device->name);
Thanks
Jiaxing
> >> return 0; >> } >> > Thanks > Barry >
| |