diff mbox series

drm: property: Replace strncpy() with strscpy_pad()

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

Commit Message

Laurent Pinchart July 31, 2021, 1:32 a.m. UTC
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(-)

Comments

Daniel Vetter Aug. 2, 2021, 8:24 a.m. UTC | #1
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 mbox series

Patch

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;