Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Double free of `field->type` string during module unload for trace events utilizing enums or sizeofs. - [Medium] Original `field->type` string allocated via `kstrdup_const` is permanently leaked when updated. --
--- Patch [1]: [PATCH v2] tracing: Fix use-after-free on field name/type of dynamic probe events --- commit b954fc1bd8487842009bed83382854061ec8a285 Author: Henry Martin <[email protected]> tracing: Fix use-after-free on field name/type of dynamic probe events Fields of a probe-based dynamic event (kprobe, uprobe and eprobe events) are created from the argument name and type strings of the trace_probe that first registers the event, as plain pointer references without copying. [ ... ] > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c > index 0356cae0cf74e..13d7675bc5132 100644 > --- a/kernel/trace/trace_events.c > +++ b/kernel/trace/trace_events.c > @@ -122,8 +122,18 @@ static int __trace_define_field(struct list_head *head, > const char *type, > if (!field) > return -ENOMEM; > > - field->name = name; > - field->type = type; > + field->name = kstrdup_const(name, GFP_TRACE); > + if (!field->name) { > + kmem_cache_free(field_cachep, field); > + return -ENOMEM; > + } > + > + field->type = kstrdup_const(type, GFP_TRACE); [Severity: Medium] Will this original field->type string be leaked when the field uses enums or sizeofs? When a module defining trace events is loaded, this kstrdup_const() allocates the string for field->type, because module rodata falls outside the core kernel rodata range. Later in the initialization sequence, trace_event_eval_update() invokes update_event_fields(), which replaces field->type without freeing the original allocation: kernel/trace/trace_events.c:update_event_fields() { ... str = kstrdup(field->type, GFP_KERNEL); ... if (call->module) add_str_to_module(call->module, str); field->type = str; ... } Should the original dynamically allocated pointer be freed before being overwritten here? > + if (!field->type) { > + kfree_const(field->name); > + kmem_cache_free(field_cachep, field); > + return -ENOMEM; > + } > > if (filter_type == FILTER_OTHER) > field->filter_type = filter_assign_type(type); [ ... ] > @@ -205,6 +215,8 @@ static void trace_destroy_fields(struct trace_event_call > *call) > head = trace_get_fields(call); > list_for_each_entry_safe(field, next, head, link) { > list_del(&field->link); > + kfree_const(field->name); > + kfree_const(field->type); [Severity: High] Could this introduce a double free during module unload for events using enums or sizeofs? During module load, update_event_fields() allocates a new string via kstrdup() and assigns it to field->type, and also adds it to the module_strings list. During module unload, trace_module_remove_events() iterates through the module's events and calls __trace_remove_event_call(), which calls trace_destroy_fields(). This newly added kfree_const(field->type) will free the string allocated by update_event_fields(). Immediately after doing that, trace_module_remove_events() iterates the module_strings list: kernel/trace/trace_events.c:trace_module_remove_events() { ... /* Check for any strings allocade for this module */ list_for_each_entry_safe(modstr, m, &module_strings, next) { if (modstr->module != mod) continue; list_del(&modstr->next); kfree(modstr->str); kfree(modstr); } ... } Will this cause kfree(modstr->str) to free the exact same string pointer a second time? > kmem_cache_free(field_cachep, field); > } > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
