linux-kernel - Re: [RFC PATCH] kernel/module: add a safer implementation of try_module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZblgV0ApD-9cQWwl@bombadil.infradead.org>
Date: Tue, 30 Jan 2024 12:47:19 -0800
From: Luis Chamberlain <mcgrof@...nel.org>
To: Marco Pagani <marpagan@...hat.com>
Cc: linux-modules@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] kernel/module: add a safer implementation of
 try_module_get()

On Tue, Jan 30, 2024 at 08:36:14PM +0100, Marco Pagani wrote:
> The current implementation of try_module_get() requires the module to
> exist and be live as a precondition. While this may seem intuitive at
> first glance, enforcing the precondition can be tricky, considering that
> modules can be unloaded at any time if not previously taken. For
> instance, the caller could be preempted just before calling
> try_module_get(), and while preempted, the module could be unloaded and
> freed. More subtly, the module could also be unloaded at any point while
> executing try_module_get() before incrementing the refount with
> atomic_inc_not_zero().
> 
> Neglecting the precondition that the module must exist and be live can
> cause unexpected race conditions that can lead to crashes. However,
> ensuring that the precondition is met may require additional locking
> that increases the complexity of the code and can make it more
> error-prone.
> 
> This patch adds a slower yet safer implementation of try_module_get()
> that checks if the module is valid by looking into the mod_tree before
> taking the module's refcount. This new function can be safely called on
> stale and invalid module pointers, relieving developers from the burden
> of ensuring that the module exists and is live before attempting to take
> it.
> 
> The tree lookup and refcount increment are executed after taking the
> module_mutex to prevent the module from being unloaded after looking up
> the tree.
> 
> Signed-off-by: Marco Pagani <marpagan@...hat.com>

It very much sounds like there is a desire to have this but without a
user, there is no justification.

> +bool try_module_get_safe(struct module *module)
> +{
> +	struct module *mod;
> +	bool ret = true;
> +
> +	if (!module)
> +		goto out;
> +
> +	mutex_lock(&module_mutex);

If a user comes around then this should be mutex_lock_interruptible(),
and add might_sleep()

> +
> +	/*
> +	 * Check if the address points to a valid live module and take
> +	 * the refcount only if it points to the module struct.
> +	 */
> +	mod = __module_address((unsigned long)module);
> +	if (mod && mod == module && module_is_live(mod))
> +		__module_get(mod);
> +	else
> +		ret = false;
> +
> +	mutex_unlock(&module_mutex);
> +
> +out:
> +	return ret;
> +}
> +EXPORT_SYMBOL(try_module_get_safe);

And EXPORT_SYMBOL_GPL() would need to be used.

I'd also expect selftests to be expanded for this case, but again,
without a user, this is just trying to resolve a problem which does not
exist.

  Luis