Re: [PATCH v2] static_call: Handle module init failure correctly in static_call_del_module()

[Jinjie Ruan]

On 2024/9/4 17:09, Thomas Gleixner wrote:
> Module insertion invokes static_call_add_module() to initialize the static
> calls in a module. static_call_add_module() invokes __static_call_init(),
> which allocates a struct static_call_mod to either encapsulate the built-in
> static call sites of the associated key into it so further modules can be
> added or to append the module to the module chain.
> 
> If that allocation fails the function returns with an error code and the
> module core invokes static_call_del_module() to clean up eventually added
> static_call_mod entries.
> 
> This works correctly, when all keys used by the module were converted over
> to a module chain before the failure. If not then static_call_del_module()
> causes a #GP as it blindly assumes that key::mods points to a valid struct
> static_call_mod.
> 
> The problem is that key::mods is not a individual struct member of struct
> static_call_key, it's part of a union to save space:
> 
>         union {
>                 /* bit 0: 0 = mods, 1 = sites */
>                 unsigned long type;
>                 struct static_call_mod *mods;
>                 struct static_call_site *sites;
> 	};
> 
> key::sites is a pointer to the list of built-in usage sites of the static
> call. The type of the pointer is differentiated by bit 0. A mods pointer
> has the bit clear, the sites pointer has the bit set.
> 
> As static_call_del_module() blidly assumes that the pointer is a valid
> static_call_mod type, it fails to check for this failure case and
> dereferences the pointer to the list of built-in call sites, which is
> obviously bogus.
> 
> Cure it by checking whether the key has a sites or a mods pointer. 
> 
> If it's a sites pointer then the key is not to be touched. As the sites are
> walked in the same order as in __static_call_init() the site walk can be
> terminated because all subsequent sites have not been touched by the init
> code due to the error exit.
> 
> If it was converted before the allocation fail, then the inner loop which
> searches for a module match will find nothing.
> 
> A fail in the second allocation in __static_call_init() is harmless and
> does not require special treatment. The first allocation succeeded and
> converted the key to a module chain. That first entry has mod::mod == NULL
> and mod::next == NULL, so the inner loop of static_call_del_module() will
> neither find a module match nor a module chain. The next site in the walk
> was either already converted, but can't match the module, or it will exit
> the outer loop because it has a static_call_site pointer and not a
> static_call_mod pointer.
> 
> Fixes: 9183c3f9ed71 ("static_call: Add inline static call infrastructure")
> Reported-by: Jinjie Ruan <ruanjinjie@xxxxxxxxxx>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Closes: https://lore.kernel.org/all/20230915082126.4187913-1-ruanjinjie@xxxxxxxxxx
> ---
> V2: Use static_call_key_has_mods() instead
> ---
>  kernel/static_call_inline.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> --- a/kernel/static_call_inline.c
> +++ b/kernel/static_call_inline.c
> @@ -411,6 +411,17 @@ static void static_call_del_module(struc
>  
>  	for (site = start; site < stop; site++) {
>  		key = static_call_key(site);
> +
> +		/*
> +		 * If the key was not updated due to a memory allocation
> +		 * failure in __static_call_init() then treating key::sites
> +		 * as key::mods in the code below would cause random memory
> +		 * access and #GP. In that case all subsequent sites have
> +		 * not been touched either, so stop iterating.
> +		 */
> +		if (!static_call_key_has_mods(key))
> +			break;
> +

Tested-by: Jinjie Ruan <ruanjinjie@xxxxxxxxxx>

>  		if (key == prev_key)
>  			continue;
>