Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [High] ddebug_match_apply_kparam() applies all dyndbg kernel parameters from the defining module indiscriminately instead of filtering for the specific classmap being matched. - [High] ddebug_sync_classbits() corrupts LEVEL_NUM classmaps by unconditionally treating the numerical verbosity level as a bitmask. - [High] Incorrect offset math in ddebug_find_valid_class() prevents dynamic debug control of offset classmaps. - [High] Out-of-bounds array read in ddebug_class_name() due to an incorrect range check. - [High] Initialization queries silently fail because the module is not yet added to the global ddebug_tables list. - [High] Unprotected read-modify-write on a shared kernel parameter in ddebug_sync_classbits(). - [Medium] TOCTOU race due to lockless access of shared parameter pointer in ddebug_apply_class_bitmap(). --
--- Patch [24]: [PATCH v6 24/24] dyndbg-API: replace DECLARE_DYNDBG_CLASSMAP --- commit 25c55a40c5e12d9e90d3e7947e7b90b45af6e5f9 Author: Jim Cromie <[email protected]> Subject: dyndbg-API: replace DECLARE_DYNDBG_CLASSMAP This commit deprecates DECLARE_DYNDBG_CLASSMAP and replaces it with DYNAMIC_DEBUG_CLASSMAP_DEFINE and DYNAMIC_DEBUG_CLASSMAP_USE. It separates classmap definitions from their usage across dependent modules like DRM drivers. This separation allows dynamic debug to initialize driver callsites independently after each is loaded. > --- 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) > { > struct ddebug_class_map *map; > + struct ddebug_class_user *cli; > int i, idx; > > - for_subvec(i, map, &dt->info, maps) { > - idx = match_string(map->class_names, map->length, class_string); > + for_subvec(i, map, di, maps) { > + idx = match_string(map->class_names, map->length, query_class); > if (idx >= 0) { > + v2pr_di_info(di, "good-class: %s.%s ", map->mod_name, > query_class); > *class_id = idx + map->base; > return map; > } > } > + 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] Does this math prevent matching against the actual callsites in the target module? When a user writes to the debugfs control file, the lookup must incorporate the offset mathematically. Subtracting the offset instead of adding it yields a mismatched or negative class ID. This will prevent finding the callsites in the user module. > + return cli->map; > + } > + } > *class_id = -ENOENT; > return NULL; > } [ ... ] > @@ -1089,12 +1112,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] Could this result in an out-of-bounds array read with a negative index? If a descriptor's actual class ID falls within the original map's un-offset range from a different use declaration, the ddebug_class_in_range() check might pass because it ignores the offset. The subsequent array access subtracts the offset, which can result in a strictly negative array index and an out-of-bounds read when reading from the debugfs control file. > + > return NULL; > } [ ... ] > @@ -1175,6 +1203,87 @@ static const struct proc_ops proc_fops = { > .proc_write = ddebug_proc_write > }; > > +#define vpr_cm_info(cm_p, msg_fmt, ...) ({ \ > + struct ddebug_class_map const *_cm = cm_p; \ > + v2pr_info(msg_fmt "%s [%d..%d] %s..%s\n", ##__VA_ARGS__, \ > + _cm->mod_name, _cm->base, _cm->base + _cm->length, \ > + _cm->class_names[0], _cm->class_names[_cm->length - 1]); \ > + }) > + > +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 unconditionally operate on the union as a bitmask, without checking the map type? When processing a DD_CLASS_TYPE_LEVEL_NUM classmap, *dcp->bits aliases *dcp->lvl. Performing a bitwise AND with CLASSMAP_BITMASK incorrectly clamps the numerical level to a bitmask value, which breaks verbosity level initialization. [Severity: High] Is this unprotected read-modify-write on a shared kernel parameter safe during concurrent module loads? When multiple dependent modules load simultaneously, they can concurrently enter ddebug_sync_classbits() and perform an unprotected bitwise AND on the shared provider parameter. This can corrupt the state if the parameter is actively being modified. > + 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); [Severity: Medium] Is there a race condition when dcp->bits is passed here and accessed locklessly? ddebug_apply_class_bitmap() iterates over the classmap length and repeatedly dereferences this pointer locklessly. Because the loop contains external function calls, the compiler must reload it. Concurrent sysfs writes could cause the loop to fetch different states on different iterations, leaving the debug state inconsistent. > + 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 != ¶m_ops_dyndbg_classes) > + return; > + > + dcp = (struct ddebug_class_param *)kp->arg; > + > + if (dcp) { [Severity: High] Does this apply all dyndbg kernel parameters from the defining module indiscriminately? When a user module references a classmap, the parameters are iterated over. Because there is no filter checking if dcp->map matches map before applying the state, it applies the state of all dyndbg parameters defined by the provider module. If class names overlap between an unused classmap and the user module's own classmaps, the user module's debug state could be incorrectly altered. > + 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); > + } > +} [ ... ] > @@ -1240,12 +1351,19 @@ static int ddebug_add_module(struct _ddebug_info *di) > */ > dt->info = *di; > dd_set_module_subrange(i, cm, &dt->info, maps); > + dd_set_module_subrange(i, cli, &dt->info, users); > + > + if (dt->info.maps.len) > + ddebug_apply_class_maps(&dt->info); > + if (dt->info.users.len) > + ddebug_apply_class_users(&dt->info); [Severity: High] Are initialization queries silently failing here because the module is not yet added to the global list? ddebug_apply_class_maps() and ddebug_apply_class_users() ultimately call ddebug_change(), which searches the global ddebug_tables list. Because the new module is added to ddebug_tables after these functions return, ddebug_change() fails to find the newly loading module. This results in zero matches, silently dropping the initial state and leaving drivers that rely on provider kernel parameters uninitialized. > > mutex_lock(&ddebug_lock); > list_add_tail(&dt->link, &ddebug_tables); > mutex_unlock(&ddebug_lock); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=24
