Re: [PATCH v8 36/43] dyndbg: resolve "protection" of class'd pr_debug

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The 'controlling_param' field in 'struct ddebug_class_map' is never assigned and its initialization path is disabled, completely breaking the intended protection mechanism for class'd pr_debugs.
--

commit 6f4d594fc681c54264d43b5caa68a2316a8b157d
Author: Jim Cromie <jim.cromie@gmail.com>

dyndbg: resolve "protection" of class'd pr_debug

This commit aims to protect class'd pr_debugs from unintended changes by
unclassed legacy queries (like `echo -p > /proc/dynamic_debug/control`).
It introduces a link from `ddebug_class_map` to `controlling_param` and
intends to check it to decide whether to protect the class map.

> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index 93a5a481c8b8c..a5d813ad323a1 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c

[ ... ]

> +static inline bool ddebug_class_has_param(const struct ddebug_class_map *map)
> +{
> +	return !!(map->controlling_param);
> +}
> +
> +/* re-framed as a policy choice */
> +#define ddebug_class_wants_protection(map) (ddebug_class_has_param(map))
> +
>  /*
>   * Search the tables for _ddebug's which match the given `query' and
>   * apply the `flags' and `mask' to them.  Returns number of matching

[ ... ]

> @@ -256,7 +299,28 @@ static bool ddebug_match_desc(const struct ddebug_query *query,
>  	    dp->lineno > query->last_lineno)
>  		return false;
>  
> -	return true;
> +	/*
> +	 * above are all satisfied, so we can make final decisions:
> +	 * 1- class FOO or implied class __DEFAULT__
> +	 * 2- site.is_classed or not
> +	 */
> +	if (query->class_string) {
> +		/* class FOO given, exact match required */
> +		return (dp->class_id == selected_class);
> +	}
> +	/* query class __DEFAULT__ by omission. */
> +	if (dp->class_id == _DPRINTK_CLASS_DFLT) {
> +		/* un-classed site */
> +		return true;
> +	}
> +	/* site is class'd */
> +	site_map = ddebug_find_map_by_class_id(di, dp->class_id);
> +	if (!site_map) {
> +		WARN_ONCE(1, "unknown class_id %d, check %s's CLASSMAP definitions", dp->class_id, di->mod_name);
> +		return false;
> +	}
> +	/* module(-param) decides protection */
> +	return !ddebug_class_wants_protection(site_map);
>  }

[Severity: Medium]
Will `ddebug_class_wants_protection()` always evaluate to false?

Because `map->controlling_param` appears to never be initialized, it seems
this will always return false, which causes the function to return true
and bypass the intended protection for class'd pr_debugs against unclassed
legacy queries.

[ ... ]

> @@ -1298,16 +1359,25 @@ 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) {
>  		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]
Was the assignment `map->controlling_param = dcp;` intended to be added here?

The commit message states that the patch "sets it in
ddebug_match_apply_kparam()", but the assignment seems to be missing.
Adding the assignment locklessly could also introduce a data race here.

[ ... ]

> @@ -1339,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
> +
>  static void ddebug_apply_class_users(const struct _ddebug_info *di)
>  {

[Severity: Medium]
Does the `#if 0` block around `ddebug_apply_class_maps()` prevent
`ddebug_match_apply_kparam()` from ever being called for the defining module?

If this function is disabled, the parameter initialization appears unreachable
for the defining module's map, even if the assignment was added to
`ddebug_match_apply_kparam()`.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905-dd-cmap-part2-clean-v8-0-a4cc0674f6fd@gmail.com?part=36