Re: [PATCH v1] clk: Free the parent map only in __clk_release()

[Jerome Brunet]

> clk_core_populate_parent_map() can fail in two ways, and since
> __clk_register() answers every failure with kref_put(&core->ref,
> __clk_release), both end in clk_core_free_parent_map() walking a parent
> map that is not there to walk:
> 
>  - If the allocation of the map fails, core->parents is NULL while
>    core->num_parents was already set from init->num_parents.
>    clk_core_free_parent_map() only tests num_parents, so it reads
>    core->parents[i].name through a NULL pointer: an oops.
> 
>  - If a later kstrdup_const() of a parent name fails, the function
>    frees the names it copied so far and the map itself, but leaves
>    core->parents pointing at the freed memory. __clk_release() then
>    frees the same names and the same map a second time: a double free
>    on top of a use-after-free read.
> 
> Both arms are the -ENOMEM path of clk_register(), clk_hw_register() and
> of_clk_hw_register(), so of every clock provider in the tree with
> num_parents > 0. A user sees it when a clock provider's probe hits a
> memory allocation failure (in practice under fault injection, as these
> allocations are small): instead of the probe failing with -ENOMEM and
> "failed to register clk", the kernel oopses in __clk_release() called
> from __clk_register(), or, for the second arm, corrupts the slab
> allocator and reports a double free.
> 
> clk_core_populate_parent_map() has cleaned up after itself since commit
> fc0c209c147f ("clk: Allow parents to be specified without string names"),
> when __clk_register()'s fail_parents label skipped
> clk_core_free_parent_map() and only the later fail_create_clk label
> freed the map. Commit 9d05ae531c2c ("clk: Initialize struct clk_core
> kref earlier") moved the freeing of everything into __clk_release() and
> pointed all failure labels at kref_put(), so the map is now freed by
> __clk_release() on this path too.
> 
> Make clk_core_populate_parent_map() leave the map to __clk_release():
> return the error without freeing anything (the map is zero-allocated and
> kfree_const(NULL) is a no-op, so the partially filled map is freed
> correctly by clk_core_free_parent_map()), and have
> clk_core_free_parent_map() return when there is no map rather than when
> num_parents is zero, which is what clk_core_get_parent_by_index()
> already does for the same state.
> 
> Found by formal verification of __clk_register() against the kernel
> binary. Reproduced on riscv64 under QEMU by re-running a fixed-factor
> clock provider's probe through its sysfs bind file with failslab aimed
> at the parent map allocation: the unpatched kernel oopses with a NULL
> pointer dereference in __clk_release() (clk_core_free_parent_map() is
> inlined there), the patched kernel fails the bind write with -ENOMEM
> and logs "failed to register clk". A userspace transcription of the
> four functions reproduces both arms under AddressSanitizer before the
> change and is clean after it. The clk KUnit suites pass before and
> after on x86_64 and riscv64.
> 
> Fixes: 9d05ae531c2c ("clk: Initialize struct clk_core kref earlier")
> Assisted-by: LLM
> Signed-off-by: Nickolai Zeldovich <nickolai@xxxxxxxxxxxxx>

Fix looks good to me. Thanks for this

The description is a touch verbose for just few lines fix, which is usual
for LLM generated content. If for some reason another version, it would
be nice you could summarize it.

Reviewed-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx>

-- 
Jerome