| Message ID | 20210731013241.11656-1-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 753f2674ad8db265986869ca07863758015deebf |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | drm: property: Replace strncpy() with strscpy_pad() | expand |
On Sat, Jul 31, 2021 at 04:32:41AM +0300, Laurent Pinchart wrote: > strncpy() is widely regarded as unsafe due to the fact that it may leave > the destination string without a nul-termination when the source string > size is too large. When compiling the kernel with W=1, the gcc warns > about this: > > drivers/gpu/drm/drm_property.c: In function ‘drm_property_create’: > drivers/gpu/drm/drm_property.c:130:2: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation] > 130 | strncpy(property->name, name, DRM_PROP_NAME_LEN); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > There are three occurrences of strncpy() in drm_property.c. None of them > are actually unsafe, as the very next line forces nul-termination of the > destination buffer. The warning is thus a false positive, but adds noise > to the kernel log. It can easily be silenced by using strscpy_pad() > instead. Do so. > > One of the three occurrences, in drm_property_add_enum(), fills a char > array that is later copied to userspace with copy_to_user() in > drm_mode_getproperty_ioctl(). To avoid leaking kernel data, > strscpy_pad() is required. Similarly, a second occurrence, in > drm_mode_getproperty_ioctl(), copies the string to an ioctl data buffer > that isn't previously zero'ed, to strscpy_pad() is also required. The > last occurrence, in drm_property_create(), would be safe to replace with > strscpy(), as the destination buffer is copied to userspace with > strscpy_pad(). However, given that this isn't in a hot path, let's avoid > future data leaks in case someone copies the whole char array blindly. +1 on just playing it safe. > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_property.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c > index 27c824a6eb60..32404891446e 100644 > --- a/drivers/gpu/drm/drm_property.c > +++ b/drivers/gpu/drm/drm_property.c > @@ -127,8 +127,7 @@ struct drm_property *drm_property_create(struct drm_device *dev, > property->num_values = num_values; > INIT_LIST_HEAD(&property->enum_list); > > - strncpy(property->name, name, DRM_PROP_NAME_LEN); > - property->name[DRM_PROP_NAME_LEN-1] = '\0'; > + strscpy_pad(property->name, name, DRM_PROP_NAME_LEN); > > list_add_tail(&property->head, &dev->mode_config.property_list); > > @@ -421,8 +420,7 @@ int drm_property_add_enum(struct drm_property *property, > if (!prop_enum) > return -ENOMEM; > > - strncpy(prop_enum->name, name, DRM_PROP_NAME_LEN); > - prop_enum->name[DRM_PROP_NAME_LEN-1] = '\0'; > + strscpy_pad(prop_enum->name, name, DRM_PROP_NAME_LEN); > prop_enum->value = value; > > property->values[index] = value; > @@ -475,8 +473,7 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, > if (!property) > return -ENOENT; > > - strncpy(out_resp->name, property->name, DRM_PROP_NAME_LEN); > - out_resp->name[DRM_PROP_NAME_LEN-1] = 0; > + strscpy_pad(out_resp->name, property->name, DRM_PROP_NAME_LEN); > out_resp->flags = property->flags; > > value_count = property->num_values; > -- > Regards, > > Laurent Pinchart >
diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index 27c824a6eb60..32404891446e 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -127,8 +127,7 @@ struct drm_property *drm_property_create(struct drm_device *dev, property->num_values = num_values; INIT_LIST_HEAD(&property->enum_list); - strncpy(property->name, name, DRM_PROP_NAME_LEN); - property->name[DRM_PROP_NAME_LEN-1] = '\0'; + strscpy_pad(property->name, name, DRM_PROP_NAME_LEN); list_add_tail(&property->head, &dev->mode_config.property_list); @@ -421,8 +420,7 @@ int drm_property_add_enum(struct drm_property *property, if (!prop_enum) return -ENOMEM; - strncpy(prop_enum->name, name, DRM_PROP_NAME_LEN); - prop_enum->name[DRM_PROP_NAME_LEN-1] = '\0'; + strscpy_pad(prop_enum->name, name, DRM_PROP_NAME_LEN); prop_enum->value = value; property->values[index] = value; @@ -475,8 +473,7 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, if (!property) return -ENOENT; - strncpy(out_resp->name, property->name, DRM_PROP_NAME_LEN); - out_resp->name[DRM_PROP_NAME_LEN-1] = 0; + strscpy_pad(out_resp->name, property->name, DRM_PROP_NAME_LEN); out_resp->flags = property->flags; value_count = property->num_values;
strncpy() is widely regarded as unsafe due to the fact that it may leave the destination string without a nul-termination when the source string size is too large. When compiling the kernel with W=1, the gcc warns about this: drivers/gpu/drm/drm_property.c: In function ‘drm_property_create’: drivers/gpu/drm/drm_property.c:130:2: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation] 130 | strncpy(property->name, name, DRM_PROP_NAME_LEN); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ There are three occurrences of strncpy() in drm_property.c. None of them are actually unsafe, as the very next line forces nul-termination of the destination buffer. The warning is thus a false positive, but adds noise to the kernel log. It can easily be silenced by using strscpy_pad() instead. Do so. One of the three occurrences, in drm_property_add_enum(), fills a char array that is later copied to userspace with copy_to_user() in drm_mode_getproperty_ioctl(). To avoid leaking kernel data, strscpy_pad() is required. Similarly, a second occurrence, in drm_mode_getproperty_ioctl(), copies the string to an ioctl data buffer that isn't previously zero'ed, to strscpy_pad() is also required. The last occurrence, in drm_property_create(), would be safe to replace with strscpy(), as the destination buffer is copied to userspace with strscpy_pad(). However, given that this isn't in a hot path, let's avoid future data leaks in case someone copies the whole char array blindly. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- drivers/gpu/drm/drm_property.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)