Re: [PATCH v6 19/24] dyndbg: pin class param storage to u32

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Support for classmaps with more than 32 classes is broken, causing undefined behavior on 32-bit architectures and silent truncation on 64-bit architectures.
--

--- Patch [19]: [PATCH v6 19/24] dyndbg: pin class param storage to u32 ---
commit df6ebc9b56fab43581ec7860d30aab6f33272ee2
Author: Jim Cromie <jim.cromie@gmail.com>

dyndbg: pin class param storage to u32

This commit changes the storage of dyndbg class parameters from unsigned long
to u32 for consistency across architectures. It also shrinks __drm_debug back
to u32.

> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index d99c69b9ad12f..af05f4ae3b55e 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
[ ... ]
> @@ -643,24 +643,27 @@ static int ddebug_apply_class_bitmap(const struct ddebug_class_param *dcp,
>  	int bi, ct;
>  
>  	if (*new_bits != old_bits)
> -		v2pr_info("apply bitmap: 0x%lx to: 0x%lx for %s\n", *new_bits,
> +		v2pr_info("apply bitmap: 0x%x to: 0x%x for %s\n", *new_bits,
>  			  old_bits, query_modname ?: "'*'");
>  
>  	for (bi = 0; bi < map->length; bi++) {
> -		if (test_bit(bi, new_bits) == test_bit(bi, &old_bits))
> +		bool new_b = !!(*new_bits & BIT(bi));
> +		bool old_b = !!(old_bits & BIT(bi));

[Severity: Medium]
Since classmaps can theoretically contain up to 63 classes (as mentioned in
the commit message), is there a risk of an out-of-bounds shift on 32-bit
architectures?

BIT(bi) expands to 1UL << bi, so if map->length is greater than 32, this
shift would exceed the capacity of an unsigned long on 32-bit systems.

On 64-bit systems, *new_bits is now a u32. When it is combined with BIT(bi)
using a bitwise AND, the u32 is zero-extended. Will this cause new_b and
old_b to always evaluate to 0 for classes >= 32, silently ignoring the
requested state?

[ ... ]
> @@ -704,24 +707,24 @@ static int param_set_dyndbg_module_classes(const char *instr,
[ ... ]
>  	case DD_CLASS_TYPE_LEVEL_NUM:
>  		/* input is bitpos, of highest verbosity to be enabled */
>  		if (inrep > map->length) {
> -			pr_warn("%s: level:%ld exceeds max:%d, clamping\n",
> +			pr_warn("%s: level:%u exceeds max:%d, clamping\n",
>  				KP_NAME(kp), inrep, map->length);
>  			inrep = map->length;
>  		}
>  		old_bits = CLASSMAP_BITMASK(*dcp->lvl);
>  		new_bits = CLASSMAP_BITMASK(inrep);

[Severity: Medium]
Will this silently prevent enablement of classes >= 32?

CLASSMAP_BITMASK was changed to limit the generated mask to 32 bits, but
map->length is not restricted to 32.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-dd-maint-2-v6-0-381f3edb0045@gmail.com?part=19