[PATCH] enclosure: bound sysfs link name construction

[PATCH] enclosure: bound sysfs link name construction

[Pengpeng Hou]

enclosure_link_name() prefixes the component device name with "enclosure_device:" in a fixed 64-byte stack buffer. The helper currently uses strcpy() and strcat() with no remaining-space check even though the component device name itself can approach the same length.

Switch the helper to bounded formatting and propagate an error when the prefixed link name does not fit.

Fixes: cb6b7f40630f ("[SCSI] ses: fix up functionality after class_device->device conversion")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/misc/enclosure.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index cf6382981777..f09707ca02e8 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -182,17 +182,21 @@ EXPORT_SYMBOL_GPL(enclosure_unregister);
 #define ENCLOSURE_NAME_SIZE	64
 #define COMPONENT_NAME_SIZE	64
 
-static void enclosure_link_name(struct enclosure_component *cdev, char *name)
+static int enclosure_link_name(struct enclosure_component *cdev, char *name)
 {
-	strcpy(name, "enclosure_device:");
-	strcat(name, dev_name(&cdev->cdev));
+	if (snprintf(name, ENCLOSURE_NAME_SIZE, "enclosure_device:%s",
+		     dev_name(&cdev->cdev)) >= ENCLOSURE_NAME_SIZE)
+		return -EINVAL;
+
+	return 0;
 }
 
 static void enclosure_remove_links(struct enclosure_component *cdev)
 {
 	char name[ENCLOSURE_NAME_SIZE];
 
-	enclosure_link_name(cdev, name);
+	if (enclosure_link_name(cdev, name))
+		return;
 
 	/*
 	 * In odd circumstances, like multipath devices, something else may
@@ -214,7 +218,12 @@ static int enclosure_add_links(struct enclosure_component *cdev)
 	if (error)
 		return error;
 
-	enclosure_link_name(cdev, name);
+	error = enclosure_link_name(cdev, name);
+	if (error) {
+		sysfs_remove_link(&cdev->cdev.kobj, "device");
+		return error;
+	}
+
 	error = sysfs_create_link(&cdev->dev->kobj, &cdev->cdev.kobj, name);
 	if (error)
 		sysfs_remove_link(&cdev->cdev.kobj, "device");
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] enclosure: bound sysfs link name construction

[Greg KH]

On Sun, Mar 29, 2026 at 11:09:43AM +0800, Pengpeng Hou wrote:
> enclosure_link_name() prefixes the component device name with "enclosure_device:" in a fixed 64-byte stack buffer. The helper currently uses strcpy() and strcat() with no remaining-space check even though the component device name itself can approach the same length.

Please wrap your changelog lines at 72 columns.

And the buffer size is fine, no need to change that logic at all, right?

> 
> Switch the helper to bounded formatting and propagate an error when the prefixed link name does not fit.
> 
> Fixes: cb6b7f40630f ("[SCSI] ses: fix up functionality after class_device->device conversion")
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>  drivers/misc/enclosure.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
> index cf6382981777..f09707ca02e8 100644
> --- a/drivers/misc/enclosure.c
> +++ b/drivers/misc/enclosure.c
> @@ -182,17 +182,21 @@ EXPORT_SYMBOL_GPL(enclosure_unregister);
>  #define ENCLOSURE_NAME_SIZE	64
>  #define COMPONENT_NAME_SIZE	64
>  
> -static void enclosure_link_name(struct enclosure_component *cdev, char *name)
> +static int enclosure_link_name(struct enclosure_component *cdev, char *name)
>  {
> -	strcpy(name, "enclosure_device:");
> -	strcat(name, dev_name(&cdev->cdev));
> +	if (snprintf(name, ENCLOSURE_NAME_SIZE, "enclosure_device:%s",
> +		     dev_name(&cdev->cdev)) >= ENCLOSURE_NAME_SIZE)
> +		return -EINVAL;

How can this ever be triggered?

> +
> +	return 0;
>  }
>  
>  static void enclosure_remove_links(struct enclosure_component *cdev)
>  {
>  	char name[ENCLOSURE_NAME_SIZE];
>  
> -	enclosure_link_name(cdev, name);
> +	if (enclosure_link_name(cdev, name))
> +		return;
>  
>  	/*
>  	 * In odd circumstances, like multipath devices, something else may
> @@ -214,7 +218,12 @@ static int enclosure_add_links(struct enclosure_component *cdev)
>  	if (error)
>  		return error;
>  
> -	enclosure_link_name(cdev, name);
> +	error = enclosure_link_name(cdev, name);
> +	if (error) {
> +		sysfs_remove_link(&cdev->cdev.kobj, "device");

This looks like an actual real bugfix, how about just sending this
portion?

thanks,

greg k-h

[PATCH v2] enclosure: bound sysfs link name construction

[Pengpeng Hou]

enclosure_link_name() prefixes the component device name with
"enclosure_device:" in a fixed 64-byte stack buffer. The helper
currently uses strcpy() and strcat() with no remaining-space check.

enclosure_component_alloc() stores component names in a 64-byte buffer
and then uses dev_set_name() on that result, so dev_name(&cdev->cdev)
can already reach 63 characters. Prefixing that with the 17-byte
"enclosure_device:" string overflows the 64-byte link-name buffer.

Use snprintf() so link-name construction stays within
ENCLOSURE_NAME_SIZE without changing the existing callers.

Fixes: cb6b7f40630f ("[SCSI] ses: fix up functionality after class_device->device conversion")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
v2:
- wrap the changelog at 72 columns
- keep the fix to bounded link-name construction only

 drivers/misc/enclosure.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/enclosure.c b/drivers/misc/enclosure.c
index cf6382981777..de457378c501 100644
--- a/drivers/misc/enclosure.c
+++ b/drivers/misc/enclosure.c
@@ -184,8 +184,8 @@ EXPORT_SYMBOL_GPL(enclosure_unregister);
 
 static void enclosure_link_name(struct enclosure_component *cdev, char *name)
 {
-	strcpy(name, "enclosure_device:");
-	strcat(name, dev_name(&cdev->cdev));
+	snprintf(name, ENCLOSURE_NAME_SIZE, "enclosure_device:%s",
+		 dev_name(&cdev->cdev));
 }
 
 static void enclosure_remove_links(struct enclosure_component *cdev)
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2] enclosure: bound sysfs link name construction

[Greg KH]

On Sun, Mar 29, 2026 at 03:39:28PM +0800, Pengpeng Hou wrote:
> enclosure_link_name() prefixes the component device name with
> "enclosure_device:" in a fixed 64-byte stack buffer. The helper
> currently uses strcpy() and strcat() with no remaining-space check.
> 
> enclosure_component_alloc() stores component names in a 64-byte buffer
> and then uses dev_set_name() on that result, so dev_name(&cdev->cdev)
> can already reach 63 characters. Prefixing that with the 17-byte
> "enclosure_device:" string overflows the 64-byte link-name buffer.
> 
> Use snprintf() so link-name construction stays within
> ENCLOSURE_NAME_SIZE without changing the existing callers.
> 
> Fixes: cb6b7f40630f ("[SCSI] ses: fix up functionality after class_device->device conversion")
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> v2:
> - wrap the changelog at 72 columns
> - keep the fix to bounded link-name construction only

That is not what I suggested that you do at all, sorry.  Please go and
re-read my last review.

thanks,

greg k-h

Re: [PATCH v2] enclosure: bound sysfs link name construction

[Pengpeng Hou]

Hi Greg,

You are right, I misread your previous review.

I took "how about just sending this portion" to mean I should keep only
the bounded-formatting part and drop the extra error propagation, but
reading your comments together now, I see that you were questioning
whether the overflow path can happen at all and were not asking for
that respin.

I should not have sent the v2 in that form.

I'll stop here rather than resend another version unless I can first
explain the trigger and align the exact patch scope.

Thanks,
Pengpeng