Re: [PATCH v7 28/29] dyndbg-API: replace DECLARE_DYNDBG_CLASSMAP

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] ddebug_sync_classbits unconditionally treats LEVEL_NUM parameters as bitmasks, corrupting stored verbosity levels and incorrectly configuring callsites.
- [High] Incorrect arithmetic for used classmaps with offsets completely breaks class control and leads to out-of-bounds array reads.
- [High] Module classmap initialization silently fails because configuration queries are executed before the module is published to the global tracking list.
- [Medium] ddebug_match_apply_kparam fails to verify that the kernel parameter's class map matches the target map, applying all loaded dyndbg parameters indiscriminately.
- [Medium] Unprotected read-modify-write on shared classmap bits causes a data race during concurrent module loads or sysfs writes.
--

--- Patch [28]: [PATCH v7 28/29] dyndbg-API: replace DECLARE_DYNDBG_CLASSMAP ---
commit bd4f2adcad3c31e1b42bccb83f503513dbc0eb2e
Author: Jim Cromie <[email protected]>
Subject: dyndbg-API: replace DECLARE_DYNDBG_CLASSMAP

This patch replaces DECLARE_DYNDBG_CLASSMAP with two new macros:
DYNAMIC_DEBUG_CLASSMAP_DEFINE and DYNAMIC_DEBUG_CLASSMAP_USE. It splits the
classmap declarations into definitions (exported by core) and uses (from
drivers) to solve initialization ordering issues and avoid duplicate classmap
allocations.

> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
[ ... ]
> @@ -160,20 +163,39 @@ static void v3pr_info_dq(const struct ddebug_query *query, const char *msg)
[ ... ]
>  static struct ddebug_class_map *ddebug_find_valid_class(struct _ddebug_info const *di,
>  							 const char *query_class,
>  							 int *class_id)
>  {
[ ... ]
> +	for_subvec(i, cli, di, users) {
> +		idx = match_string(cli->map->class_names, cli->map->length, query_class);
> +		if (idx >= 0) {
> +			v2pr_di_info(di, "class-ref: %s -> %s.%s ",
> +				    cli->mod_name, cli->map->mod_name, query_class);
> +			*class_id = idx + cli->map->base - cli->offset;

[Severity: High]
Should this subtract the offset? When a module uses a classmap with a non-zero
offset, subtracting it here instead of adding it appears to yield the wrong
class ID, breaking sysfs control ID mapping.

> +			return cli->map;
> +		}
> +	}
> +	*class_id = -ENOENT;
> +	return NULL;
>  }
[ ... ]
> @@ -1090,12 +1113,17 @@ static bool ddebug_class_in_range(const int class_id, const struct ddebug_class_
>  static const char *ddebug_class_name(struct _ddebug_info *di, struct _ddebug *dp)
>  {
>  	struct ddebug_class_map *map;
> +	struct ddebug_class_user *cli;
>  	int i;
>  
>  	for_subvec(i, map, di, maps)
>  		if (ddebug_class_in_range(dp->class_id, map))
>  			return map->class_names[dp->class_id - map->base];
>  
> +	for_subvec(i, cli, di, users)
> +		if (ddebug_class_in_range(dp->class_id, cli->map))
> +			return cli->map->class_names[dp->class_id - cli->map->base - cli->offset];

[Severity: High]
Does this compute a negative array index? If an unrelated callsite falls in
the unadjusted range checked by ddebug_class_in_range(), subtracting both
base and offset from dp->class_id could result in a negative index. This
could lead to an out-of-bounds read on cli->map->class_names.

> +
>  	return NULL;
>  }
[ ... ]
> @@ -1176,6 +1204,87 @@ static const struct proc_ops proc_fops = {
[ ... ]
> +static void ddebug_sync_classbits(const struct kernel_param *kp, const char *modname)
> +{
> +	const struct ddebug_class_param *dcp = kp->arg;
> +
> +	/* clamp initial bitvec, mask off hi-bits */
> +	if (*dcp->bits & ~CLASSMAP_BITMASK(dcp->map->length)) {
> +		*dcp->bits &= CLASSMAP_BITMASK(dcp->map->length);

[Severity: High]
Does this logic corrupt LEVEL_NUM classmaps? By unconditionally treating the
union field *dcp->bits as a bitmask, this overwrites the integer level stored
in *dcp->lvl for DD_CLASS_TYPE_LEVEL_NUM. Treating an integer level as a
bitmask seems to incorrectly enable disjoint classes and corrupt the stored
verbosity level.

[Severity: Medium]
Could this unprotected read-modify-write on *dcp->bits cause a data race?
Since this can execute during concurrent module loads or sysfs writes without
ddebug_lock, it looks like concurrent updates to the shared parent classmap
bitmask could be lost.

> +		v2pr_info("preset classbits: %x\n", *dcp->bits);
> +	}
> +	/* force class'd prdbgs (in USEr module) to match (DEFINEr module) class-param */
> +	ddebug_apply_class_bitmap(dcp, dcp->bits, ~0, modname);
> +	ddebug_apply_class_bitmap(dcp, dcp->bits, 0, modname);
> +}
> +
> +static void ddebug_match_apply_kparam(const struct kernel_param *kp,
> +				      const struct ddebug_class_map *map,
> +				      const char *mod_name)
> +{
> +	struct ddebug_class_param *dcp;
> +
> +	if (kp->ops != &param_ops_dyndbg_classes)
> +		return;
> +
> +	dcp = (struct ddebug_class_param *)kp->arg;
> +
> +	if (dcp) {
> +		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);

[Severity: Medium]
Is a check comparing dcp->map to map missing here? Because this verifies
kp->ops but omits verifying that the kernel parameter's class map matches the
target map, it seems all dyndbg parameters of the defining module will be
applied to every classmap indiscriminately.

> +	}
> +}
[ ... ]
> @@ -1213,6 +1322,7 @@ static int ddebug_add_module(struct _ddebug_info *di)
[ ... ]
> +	if (dt->info.maps.len)
> +		ddebug_apply_class_maps(&dt->info);
> +	if (dt->info.users.len)
> +		ddebug_apply_class_users(&dt->info);
>  
>  	mutex_lock(&ddebug_lock);
>  	list_add_tail(&dt->link, &ddebug_tables);
>  	mutex_unlock(&ddebug_lock);

[Severity: High]
Will module classmap initialization fail here? The initialization functions
ddebug_apply_class_maps() and ddebug_apply_class_users() execute queries
against the global ddebug_tables list. Since list_add_tail() is called
after these functions return, it appears ddebug_change() will fail to find
the newly loaded module, silently dropping the initial configuration.

>  
> -	vpr_info("%3u debug prints in module %s\n", di->descs.len, di->mod_name);
> +	vpr_info("%3u debug prints in module %s\n",
> +		 dt->info.descs.len, dt->info.mod_name);
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=28