Re: [PATCH v9 33/39] dyndbg: resolve "protection" of class'd pr_debug

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The function `ddebug_apply_class_maps` is enclosed in an `#if 0` block, preventing `controlling_param` from being initialized for defining modules. This breaks the protection mechanism for parameterized classmaps and fails to apply boot/load-time kernel parameters to the defining module's callsites.
- [Medium] The module is exposed to userspace queries via `ddebug_tables` before its `map->controlling_param` protection state is initialized, creating a data race and TOCTOU vulnerability where concurrent classless queries can bypass protection.
--

commit 3a846cbf484a221d77612af005e27faf83c306df
Author: Jim Cromie <jim.cromie@gmail.com>

dyndbg: resolve "protection" of class'd pr_debug

This commit adds protection for dynamically debugged classes against
class-less queries by using sysfs parameters. It sets a controlling
parameter for parameterized classmaps to prevent accidental modification
from queries that lack a specific class identifier.

> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index e6f5a524d63b6..8c90b1e4cd33e 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c

[ ... ]

> @@ -1297,25 +1358,35 @@ static void ddebug_sync_classbits(const struct kernel_param *kp, const char *mod
>  	}
>  }
>  
> -static void ddebug_match_apply_kparam(const struct kernel_param *kp,
> -				      const struct ddebug_class_map *map,
> -				      const char *mod_name)
> +static struct ddebug_class_param *
> +ddebug_get_classmap_kparam(const struct kernel_param *kp,
> +			   const struct ddebug_class_map *map)
>  {
>  	struct ddebug_class_param *dcp;
>  
>  	if (kp->ops != &param_ops_dyndbg_classes)
> -		return;
> +		return NULL;
>  
>  	dcp = (struct ddebug_class_param *)kp->arg;
> +	return (map == dcp->map)
> +		? dcp : (struct ddebug_class_param *)NULL;
> +}
> +
> +static void ddebug_match_apply_kparam(const struct kernel_param *kp,
> +				      struct ddebug_class_map *map,
> +				      const char *mod_name)
> +{
> +	struct ddebug_class_param *dcp = ddebug_get_classmap_kparam(kp, map);
>  
>  	if (dcp && dcp->map == map) {
> +		map->controlling_param = dcp;

[Severity: Medium]
Could this lockless write to map->controlling_param cause a data race?

When a module is loaded, ddebug_add_module() adds the module to the
ddebug_tables list under the ddebug_lock:

mutex_lock(&ddebug_lock);
list_add_tail(&dt->link, &ddebug_tables);
mutex_unlock(&ddebug_lock);

It then drops the lock and calls ddebug_apply_class_users(), which reaches
this code and locklessly writes to map->controlling_param.

Since the module is already visible in ddebug_tables before this
initialization completes, could a concurrent classless sysfs query bypass
protection by evaluating ddebug_class_wants_protection() on a newly added
module before controlling_param is set?

> +		v2pr_info(" kp:%s.%s =0x%x", mod_name, kp->name, *dcp->bits);
> +		vpr_cm_info(map, " %s maps ", mod_name);
> +		ddebug_sync_classbits(kp, mod_name);
>  	}
>  }

[ ... ]

> @@ -1338,6 +1409,26 @@ static void ddebug_apply_params(const struct ddebug_class_map *cm, const char *m
>  	}
>  }
>  
> +#if 0
> +/*
> + * called from add_module, ie early. it can find controlling kparams,
> + * which can/does? enable protection of this classmap from class-less
> + * queries, on the grounds that the user created the kparam, means to
> + * use it, and expects it to reflect reality.  We should oblige him,
> + * and protect those classmaps from classless "-p" changes.
> + */
> +static void ddebug_apply_class_maps(const struct _ddebug_info *di)
> +{
> +	struct ddebug_class_map *cm;
> +	int i;
> +
> +	for_subvec(i, cm, di, maps)
> +		ddebug_apply_params(cm, cm->mod_name);
> +
> +	v2pr_di_info(di, "attached %d class-maps to ", i);
> +}
> +#endif

[Severity: High]
Does this disabled code block break the intended protection for parameterized
classmaps defined within the module?

Because ddebug_apply_class_maps() is enclosed in an #if 0 block, the
controlling_param is never initialized for the module defining the classmap.
As a result, ddebug_class_wants_protection() will evaluate to false because
site_map->controlling_param is NULL.

Will this cause boot and load-time kernel parameters (like drm.debug=VAL)
to not be applied to the defining module's callsites, allowing class-less
queries to improperly modify them?

> +
>  static void ddebug_apply_class_users(const struct _ddebug_info *di)
>  {
>  	struct ddebug_class_user *cli;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-dd-cmap-part2-clean-v9-0-3a3f621f4a10@gmail.com?part=33