Re: [Linuxarm] [PATCH v1] drm/nouveau/device: append a NUL-terminated character for the string which filled by strncpy()

[luojiaxing]

On 2021/2/26 9:01, Song Bao Hua (Barry Song) wrote:
> > -----Original Message-----
> > From: Luo Jiaxing [mailto:luojiaxing@xxxxxxxxxx]
> > Sent: Friday, February 26, 2021 12:39 AM
> > To: nouveau@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> > bskeggs@xxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx; linuxarm@xxxxxxxxxxxxx; luojiaxing
> > <luojiaxing@xxxxxxxxxx>
> > 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@xxxxxxxxxx>
> > ---
> >   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