Message-ID: <512be61587339db248f5d88dc5240ee3.sboyd@kernel.org>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20230206083305.147582-1-sergio.paracuellos@gmail.com>
References: <20230206083305.147582-1-sergio.paracuellos@gmail.com>
Subject: Re: [PATCH] clk: ralink: fix 'mt7621_gate_is_enabled()' function
From:   Stephen Boyd <sboyd@kernel.org>
Cc:     linux-kernel@vger.kernel.org, mturquette@baylibre.com,
        matthias.bgg@gmail.com
To:     Sergio Paracuellos <sergio.paracuellos@gmail.com>,
        linux-clk@vger.kernel.org
Date:   Fri, 10 Feb 2023 16:07:37 -0800
User-Agent: alot/0.10
Precedence: bulk

Quoting Sergio Paracuellos (2023-02-06 00:33:05)
> Compiling clock driver with CONFIG_UBSAN enabled shows the following trac=
e:
>=20
> UBSAN: shift-out-of-bounds in drivers/clk/ralink/clk-mt7621.c:121:15
> shift exponent 131072 is too large for 32-bit type 'long unsigned int'
> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.15.86 #0
> Stack : ...
>=20
> Call Trace:
> [<80009a58>] show_stack+0x38/0x118
> [<8045ce04>] dump_stack_lvl+0x60/0x80
> [<80458868>] ubsan_epilogue+0x10/0x54
> [<804590e0>] __ubsan_handle_shift_out_of_bounds+0x118/0x190
> [<804c9a10>] mt7621_gate_is_enabled+0x98/0xa0
> [<804bb774>] clk_core_is_enabled+0x34/0x90
> [<80aad73c>] clk_disable_unused_subtree+0x98/0x1e4
> [<80aad6d4>] clk_disable_unused_subtree+0x30/0x1e4
> [<80aad6d4>] clk_disable_unused_subtree+0x30/0x1e4
> [<80aad900>] clk_disable_unused+0x78/0x120
> [<80002030>] do_one_initcall+0x54/0x1f0
> [<80a922a4>] kernel_init_freeable+0x280/0x31c
> [<808047c4>] kernel_init+0x20/0x118
> [<80003e58>] ret_from_kernel_thread+0x14/0x1c
>=20
> Shifting a value (131032) larger than the type (32 bit unsigned integer)
> is undefined behaviour in C.
>=20
> The problem is in 'mt7621_gate_is_enabled()' function which is using the
> 'BIT()' kernel macro with the bit index for the clock gate to check if the
> bit is set. When the clock gates structure is created driver is already
> setting 'bit_idx' using 'BIT()' macro, so we are wrongly applying an extra
> 'BIT()' mask here. Removing it solve the problem and makes this function
> correct. However when clock gating is correctly working, the kernel starts
> disabling those clocks that are not requested. Some drivers for this SoC
> are older than this clock driver itself. So to avoid the kernel to disable
> clocks that have been enabled until now, we must apply 'CLK_IS_CRITICAL'
> flag on gates initialization code.
>=20
> Fixes: 48df7a26f470 ("clk: ralink: add clock driver for mt7621 SoC")
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---

Applied to clk-next