[PATCH] drm/nouveau/bl: Use kasprintf for interface name

[PATCH] drm/nouveau/bl: Use kasprintf for interface name

[Jacob Keller]

The nouveau_get_backlight_name() function generates a unique name for the
backlight interface, appending an id from 1 to 99 for all backlight devices
after the first.

GCC 15 (and likely other compilers) produce the following
-Wformat-truncation warning:

nouveau_backlight.c: In function ‘nouveau_backlight_init’:
nouveau_backlight.c:56:69: error: ‘%d’ directive output may be truncated writing between 1 and 10 bytes into a region of size 3 [-Werror=format-truncation=]
   56 |                 snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight%d", nb);
      |                                                                     ^~
In function ‘nouveau_get_backlight_name’,
    inlined from ‘nouveau_backlight_init’ at nouveau_backlight.c:351:7:
nouveau_backlight.c:56:56: note: directive argument in the range [1, 2147483647]
   56 |                 snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight%d", nb);
      |                                                        ^~~~~~~~~~~~~~~~
nouveau_backlight.c:56:17: note: ‘snprintf’ output between 14 and 23 bytes into a destination of size 15
   56 |                 snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight%d", nb);
      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The warning started appearing after commit ab244be47a8f ("drm/nouveau:
Fix a potential theorical leak in nouveau_get_backlight_name()") This fix
for the ida usage removed the explicit value check for ids larger than 99.
The compiler is unable to intuit that the ida_alloc_max() limits the
returned value range between 0 and 99.

The warning has gone unfixed for some time, with at least one kernel test
robot report. The code breaks W=1 builds, which is especially frustrating
with the introduction of CONFIG_WERROR.

Refactor the function to avoid the fixed-length buffer entirely. Use
kasprintf to allocate a buffer of appropriate size. This avoids the need
for BL_NAME_SIZE and resolves the -Wformat-truncation warning, fixing W=1
builds.

Compile tested only.

Fixes: ab244be47a8f ("drm/nouveau: Fix a potential theorical leak in nouveau_get_backlight_name()")
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202312050324.0kv4PnfZ-lkp@intel.com/
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
This could also be fixed by simply increasing BL_NAME_SIZE to 24, making it
large enough to fit any size integer into its format string, or by checking
the return value of snprintf.
---
 drivers/gpu/drm/nouveau/nouveau_backlight.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c b/drivers/gpu/drm/nouveau/nouveau_backlight.c
index d47442125fa183146135f3725eae161c68e2a900..d3cf69fe2eeb33c24ee698db19b34a56a72f51cb 100644
--- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
+++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
@@ -42,22 +42,21 @@
 #include "nouveau_acpi.h"
 
 static struct ida bl_ida;
-#define BL_NAME_SIZE 15 // 12 for name + 2 for digits + 1 for '\0'
 
-static bool
-nouveau_get_backlight_name(char backlight_name[BL_NAME_SIZE],
-			   struct nouveau_backlight *bl)
+static char *
+nouveau_get_backlight_name(struct nouveau_backlight *bl)
 {
 	const int nb = ida_alloc_max(&bl_ida, 99, GFP_KERNEL);
 
 	if (nb < 0)
-		return false;
-	if (nb > 0)
-		snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight%d", nb);
-	else
-		snprintf(backlight_name, BL_NAME_SIZE, "nv_backlight");
+		return NULL;
+
 	bl->id = nb;
-	return true;
+
+	if (nb > 0)
+		return kasprintf(GFP_KERNEL, "nv_backlight%d", nb);
+	else
+		return kasprintf(GFP_KERNEL, "nv_backlight");
 }
 
 static int
@@ -293,9 +292,9 @@ nouveau_backlight_init(struct drm_connector *connector)
 	struct nouveau_backlight *bl;
 	struct nouveau_encoder *nv_encoder = NULL;
 	struct nvif_device *device = &drm->client.device;
-	char backlight_name[BL_NAME_SIZE];
 	struct backlight_properties props = {0};
 	const struct backlight_ops *ops;
+	char *backlight_name;
 	int ret;
 
 	if (apple_gmux_present()) {
@@ -348,7 +347,8 @@ nouveau_backlight_init(struct drm_connector *connector)
 		goto fail_alloc;
 	}
 
-	if (!nouveau_get_backlight_name(backlight_name, bl)) {
+	backlight_name = nouveau_get_backlight_name(bl);
+	if (!backlight_name) {
 		NV_ERROR(drm, "Failed to retrieve a unique name for the backlight interface\n");
 		goto fail_alloc;
 	}
@@ -356,6 +356,7 @@ nouveau_backlight_init(struct drm_connector *connector)
 	props.type = BACKLIGHT_RAW;
 	bl->dev = backlight_device_register(backlight_name, connector->kdev,
 					    nv_encoder, ops, &props);
+	kfree(backlight_name);
 	if (IS_ERR(bl->dev)) {
 		if (bl->id >= 0)
 			ida_free(&bl_ida, bl->id);

---
base-commit: 90b83efa6701656e02c86e7df2cb1765ea602d07
change-id: 20250604-jk-nouveua-drm-bl-snprintf-fix-c2ca525a3d2f

Best regards,
-- 
Jacob Keller <jacob.e.keller@intel.com>

Re: [PATCH] drm/nouveau/bl: Use kasprintf for interface name

[Timur Tabi]

On Wed, 2025-06-04 at 14:03 -0700, Jacob Keller wrote:
> This could also be fixed by simply increasing BL_NAME_SIZE to 24, making it
> large enough to fit any size integer into its format string, or by checking
> the return value of snprintf.

I think this would be a better approach.  backlist_name[24] is still small enough to fit on the
stack with ease, and it would eliminate the alloc/free churn.  kasprintf() should be reserved for
long-term or larger allocations, IMHO.

Re: [PATCH] drm/nouveau/bl: Use kasprintf for interface name

[Jacob Keller]

On 6/4/2025 3:01 PM, Timur Tabi wrote:
> On Wed, 2025-06-04 at 14:03 -0700, Jacob Keller wrote:
>> This could also be fixed by simply increasing BL_NAME_SIZE to 24, making it
>> large enough to fit any size integer into its format string, or by checking
>> the return value of snprintf.
> 
> I think this would be a better approach.  backlist_name[24] is still small enough to fit on the
> stack with ease, and it would eliminate the alloc/free churn.  kasprintf() should be reserved for
> long-term or larger allocations, IMHO.

Fair enough. Another alternative which saves the stack and macro would
be to add a new version of backlight_register_device which can pass the
integer number itself into dev_set_name, but thats probably not worth
the churn either. (Currently we snprintf into a buffer and then again
copy that again into the device name...)

Thanks,
Jake

Re: [PATCH] drm/nouveau/bl: Use kasprintf for interface name

[Timur Tabi]

On Wed, 2025-06-04 at 15:16 -0700, Jacob Keller wrote:
> Fair enough. Another alternative which saves the stack and macro would
> be to add a new version of backlight_register_device which can pass the
> integer number itself into dev_set_name, but thats probably not worth
> the churn either. (Currently we snprintf into a buffer and then again
> copy that again into the device name...)

If you really wanted to avoid stack space, you could add backlist_name[24] to 'struct
nouveau_backlight'.  

Re: [PATCH] drm/nouveau/bl: Use kasprintf for interface name

[Jacob Keller]

On 6/4/2025 3:40 PM, Timur Tabi wrote:
> On Wed, 2025-06-04 at 15:16 -0700, Jacob Keller wrote:
>> Fair enough. Another alternative which saves the stack and macro would
>> be to add a new version of backlight_register_device which can pass the
>> integer number itself into dev_set_name, but thats probably not worth
>> the churn either. (Currently we snprintf into a buffer and then again
>> copy that again into the device name...)
> 
> If you really wanted to avoid stack space, you could add backlist_name[24] to 'struct
> nouveau_backlight'.  
> 

I'm not too worried about it. I'll wait a bit for anyone else to comment
and then send a v2 with blacklist_name set to 24 bytes.

Thanks,
Jake