Nouveau - Feb 2022 - [PATCH] drm/nouveau: Fix a potential theorical leak in nouveau_get_backlight_name()

If successful ida_simple_get() calls are not undone when needed, some
additional memory may be allocated and wasted.

Here, an ID between 0 and MAX_INT is required. If this ID is >=100, it is
not taken into account and is wasted. It should be released.

Instead of calling ida_simple_remove(), take advantage of the 'max'
parameter to require the ID not to be too big. Should it be too big, it
is not allocated and don't need to be freed.

While at it, use ida_alloc_xxx()/ida_free() instead to
ida_simple_get()/ida_simple_remove().
The latter is deprecated and more verbose.

Fixes: db1a0ae21461 ("drm/nouveau/bl: Assign different names to
interfaces")
Signed-off-by: Christophe JAILLET <christophe.jaillet at wanadoo.fr>
---
This patch is more a clean-up than a fix.
It is unlikely than >= 100 backlight devices will be registered, and the
over allocation would occur even much later when the underlying xarray is
full.

I also think that the 'if (bl->id >= 0)' before the
ida_simple_remove()
calls are useless. We don't store the id if a negative (i.e. error) is
returned by ida_simple_get().

Finally, having a '#define BL_MAX_MINORS 99' could be better than a
magic number in the code.
---
 drivers/gpu/drm/nouveau/nouveau_backlight.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c
b/drivers/gpu/drm/nouveau/nouveau_backlight.c
index ae2f2abc8f5a..ccd080ba30bf 100644
--- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
+++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
@@ -46,8 +46,8 @@ static bool
 nouveau_get_backlight_name(char backlight_name[BL_NAME_SIZE],
 			   struct nouveau_backlight *bl)
 {
-	const int nb = ida_simple_get(&bl_ida, 0, 0, GFP_KERNEL);
-	if (nb < 0 || nb >= 100)
+	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);
@@ -414,7 +414,7 @@ nouveau_backlight_init(struct drm_connector *connector)
 					    nv_encoder, ops, &props);
 	if (IS_ERR(bl->dev)) {
 		if (bl->id >= 0)
-			ida_simple_remove(&bl_ida, bl->id);
+			ida_free(&bl_ida, bl->id);
 		ret = PTR_ERR(bl->dev);
 		goto fail_alloc;
 	}
@@ -442,7 +442,7 @@ nouveau_backlight_fini(struct drm_connector *connector)
 		return;
 
 	if (bl->id >= 0)
-		ida_simple_remove(&bl_ida, bl->id);
+		ida_free(&bl_ida, bl->id);
 
 	backlight_device_unregister(bl->dev);
 	nv_conn->backlight = NULL;
-- 
2.32.0

Sorry I totally missed this patch up until now, noticed it while going through
unread emails today. This is:

Reviewed-by: Lyude Paul <lyude at redhat.com>

FWIW, if there's something you need reviews on that hasn't gotten looked
at
after a few weeks feel free to poke the nouveau list/me.

Anyway, I will go ahead and push this to drm-misc-fixes in a moment. Thanks!

On Wed, 2022-02-09 at 07:03 +0100, Christophe JAILLET
wrote:
> If successful ida_simple_get() calls are not undone when needed, some
> additional memory may be allocated and wasted.
> 
> Here, an ID between 0 and MAX_INT is required. If this ID is >=100, it
is
> not taken into account and is wasted. It should be released.
> 
> Instead of calling ida_simple_remove(), take advantage of the 'max'
> parameter to require the ID not to be too big. Should it be too big, it
> is not allocated and don't need to be freed.
> 
> While at it, use ida_alloc_xxx()/ida_free() instead to
> ida_simple_get()/ida_simple_remove().
> The latter is deprecated and more verbose.
> 
> Fixes: db1a0ae21461 ("drm/nouveau/bl: Assign different names to
interfaces")
> Signed-off-by: Christophe JAILLET <christophe.jaillet at wanadoo.fr>
> ---
> This patch is more a clean-up than a fix.
> It is unlikely than >= 100 backlight devices will be registered, and the
> over allocation would occur even much later when the underlying xarray is
> full.
> 
> I also think that the 'if (bl->id >= 0)' before the
ida_simple_remove()
> calls are useless. We don't store the id if a negative (i.e. error) is
> returned by ida_simple_get().
> 
> Finally, having a '#define BL_MAX_MINORS 99' could be better than a
> magic number in the code.
> ---
> ?drivers/gpu/drm/nouveau/nouveau_backlight.c | 8 ++++----
> ?1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c
> b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> index ae2f2abc8f5a..ccd080ba30bf 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
> @@ -46,8 +46,8 @@ static bool
> ?nouveau_get_backlight_name(char backlight_name[BL_NAME_SIZE],
> ?????????????????????????? struct nouveau_backlight *bl)
> ?{
> -???????const int nb = ida_simple_get(&bl_ida, 0, 0, GFP_KERNEL);
> -???????if (nb < 0 || nb >= 100)
> +???????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);
> @@ -414,7 +414,7 @@ nouveau_backlight_init(struct drm_connector *connector)
> ??????????????????????????????????????????? nv_encoder, ops, &props);
> ????????if (IS_ERR(bl->dev)) {
> ????????????????if (bl->id >= 0)
> -???????????????????????ida_simple_remove(&bl_ida, bl->id);
> +???????????????????????ida_free(&bl_ida, bl->id);
> ????????????????ret = PTR_ERR(bl->dev);
> ????????????????goto fail_alloc;
> ????????}
> @@ -442,7 +442,7 @@ nouveau_backlight_fini(struct drm_connector *connector)
> ????????????????return;
> ?
> ????????if (bl->id >= 0)
> -???????????????ida_simple_remove(&bl_ida, bl->id);
> +???????????????ida_free(&bl_ida, bl->id);
> ?
> ????????backlight_device_unregister(bl->dev);
> ????????nv_conn->backlight = NULL;
-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat