Re: [PATCH v2 18/24] dyndbg: Upgrade class param storage to u64 for 64-bit classmaps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Buffer Overflow / Memory Corruption due to mismatched types in `ddebug_class_param`.

Pre-existing issues:
- [High] Undefined Behavior in `CLASSMAP_BITMASK` triggers shift overflow for 64-bit classmaps.
--

commit 58d2c659362026860249b9d71580382d0d84aa4e
Author: Jim Cromie <jim.cromie@gmail.com>

dyndbg: Upgrade class param storage to u64 for 64-bit classmaps

This commit upgrades the internal storage types in struct
ddebug_class_param from unsigned long to u64 pointers. This change
aims to safely support 64-bit classmaps across all architectures
without truncation.

> diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
> index e9481ef21825d..580aa258f902f 100644
> --- a/lib/dynamic_debug.c
> +++ b/lib/dynamic_debug.c
> @@ -661,7 +663,7 @@ static int ddebug_apply_class_bitmap(const struct ddebug_class_param *dcp,
>  /* stub to later conditionally add "$module." prefix where not already done */
>  #define KP_NAME(kp)	kp->name
>  
> -#define CLASSMAP_BITMASK(width) ((1UL << (width)) - 1)
> +#define CLASSMAP_BITMASK(width) ((1ULL << (width)) - 1)

[Severity: High]
This isn't a bug introduced by this patch, but does this macro invoke
undefined behavior if the classmap width reaches 64 bits?

If width is 64, shifting 1ULL by 64 is undefined behavior in C. On some
architectures like x86, the shift count is masked by 63, causing 1ULL << 64
to evaluate to 1ULL << 0 (which is 1). This would result in a bitmask of 0,
wiping out valid input bits and breaking the 64-bit feature.

[ ... ]

> @@ -696,24 +698,24 @@ static int param_set_dyndbg_module_classes(const char *instr,
>  	case DD_CLASS_TYPE_DISJOINT_BITS:
>  		/* expect bits. mask and warn if too many */
>  		if (inrep & ~CLASSMAP_BITMASK(map->length)) {
> -			pr_warn("%s: input: 0x%lx exceeds mask: 0x%lx, masking\n",
> +			pr_warn("%s: input: 0x%llx exceeds mask: 0x%llx, masking\n",
>  				KP_NAME(kp), inrep, CLASSMAP_BITMASK(map->length));
>  			inrep &= CLASSMAP_BITMASK(map->length);
>  		}
> -		v2pr_info("bits:0x%lx > %s.%s\n", inrep, mod_name ?: "*", KP_NAME(kp));
> +		v2pr_info("bits:0x%llx > %s.%s\n", inrep, mod_name ?: "*", KP_NAME(kp));
>  		totct += ddebug_apply_class_bitmap(dcp, &inrep, *dcp->bits, mod_name);
>  		*dcp->bits = inrep;

[Severity: High]
Can this cause memory corruption on 32-bit architectures?

The commit message notes that __drm_debug is not yet updated to u64,
meaning it remains an unsigned long. On 32-bit systems, unsigned long is 4
bytes. Since dcp->bits is now a u64 pointer, won't assigning inrep perform
an 8-byte write into a 4-byte allocation, corrupting adjacent memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260523-dd-maint-2-v2-0-b937312aa083@gmail.com?part=18