linux-kernel - [heads-up] Re: [PATCH] reset: Further simplify locking with guard()

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] [day] [month] [year] [list]

Message-ID: <20240928222702.GX3550746@ZenIV>
Date: Sat, 28 Sep 2024 23:27:02 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Philipp Zabel <p.zabel@...gutronix.de>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	kernel@...gutronix.de,
	Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Peter Zijlstra <peterz@...radead.org>
Subject: [heads-up] Re: [PATCH] reset: Further simplify locking with guard()

On Fri, Sep 27, 2024 at 04:02:32PM +0200, Philipp Zabel wrote:
> Use guard(mutex) to automatically unlock mutexes when going out of
> scope. Simplify error paths by removing a goto and manual mutex
> unlocking in multiple places.

And that, folks, is a live example of the reasons why guard() is an
attractive nuisance.  We really need a very loud warning on
cleanup.h stuff - otherwise such patches from well-meaning folks
will keep coming.

> @@ -1041,29 +1036,27 @@ __of_reset_control_get(struct device_node *node, const char *id, int index,
>  		}
>  	}
>  
> -	mutex_lock(&reset_list_mutex);
> +	guard(mutex)(&reset_list_mutex);
>  	rcdev = __reset_find_rcdev(&args, gpio_fallback);
>  	if (!rcdev) {
>  		rstc = ERR_PTR(-EPROBE_DEFER);
> -		goto out_unlock;
> +		goto out_put;
>  	}
>  
>  	if (WARN_ON(args.args_count != rcdev->of_reset_n_cells)) {
>  		rstc = ERR_PTR(-EINVAL);
> -		goto out_unlock;
> +		goto out_put;
>  	}
>  
>  	rstc_id = rcdev->of_xlate(rcdev, &args);
>  	if (rstc_id < 0) {
>  		rstc = ERR_PTR(rstc_id);
> -		goto out_unlock;
> +		goto out_put;
>  	}
>  
>  	/* reset_list_mutex also protects the rcdev's reset_control list */
>  	rstc = __reset_control_get_internal(rcdev, rstc_id, shared, acquired);
>  
> -out_unlock:
> -	mutex_unlock(&reset_list_mutex);
>  out_put:
>  	of_node_put(args.np);

Guess what happens if you take goto out_put prior to the entire thing,
in
                ret = __reset_add_reset_gpio_device(&args);
		if (ret) {
			rstc = ERR_PTR(ret);
			goto out_put;
		}
That patch adds implicit mutex_unlock() at the points where we leave
the scope.  Which extends to the end of function.  In other words, there is
one downstream of out_put, turning any goto out_put upstream of guard() into
a bug.

What's worse, that bug is not caught by gcc - it quietly generates bogus code
that will get unnoticed until we get an error from __reset_add_reset_gpio_device()
call.  At that point we'll look at the contents of uninitialized variable and,
if we are unlucky, call mutex_unlock() (with hell knows what pointer passed to it -
not that mutex_unlock(&reset_list_mutex) would do us any good at that point, since
we hadn't locked it in the first place).

Folks, that kind of cleanup patches is bloody dangerous; even something that
currently avoids that crap can easily grow that kind of quiet breakage later.