linux-kernel - Re: [PATCH] clkdev: report over-sized strings when creating clkdev entries

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]

Date: Fri, 17 May 2024 20:24:19 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: linux-arm-kernel@...ts.infradead.org,
 Duanqiang Wen <duanqiangwen@...-swift.com>, mturquette@...libre.com,
 sboyd@...nel.org, linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] clkdev: report over-sized strings when creating clkdev
 entries

On 5/17/24 16:37, Russell King (Oracle) wrote:
> On Fri, May 17, 2024 at 04:34:06PM -0700, Guenter Roeck wrote:
>> On 5/17/24 15:22, Russell King (Oracle) wrote:
>>> On Fri, May 17, 2024 at 03:09:12PM -0700, Guenter Roeck wrote:
>>>> Hi,
>>>>
>>>> On Fri, Mar 15, 2024 at 11:47:55AM +0000, Russell King (Oracle) wrote:
>>>>> Report an error when an attempt to register a clkdev entry results in a
>>>>> truncated string so the problem can be easily spotted.
>>>>>
>>>>> Reported by: Duanqiang Wen <duanqiangwen@...-swift.com>
>>>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
>>>>> Reviewed-by: Stephen Boyd <sboyd@...nel.org>
>>>>
>>>> With this patch in the mainline kernel, I get
>>>>
>>>> 10000000.clock-controller:corepll: device ID is greater than 24
>>>> sifive-clk-prci 10000000.clock-controller: Failed to register clkdev for corepll: -12
>>>> sifive-clk-prci 10000000.clock-controller: could not register clocks: -12
>>>> sifive-clk-prci 10000000.clock-controller: probe with driver sifive-clk-prci failed with error -12
>>>> ...
>>>> platform 10060000.gpio: deferred probe pending: platform: supplier 10000000.clock-controller not ready
>>>> platform 10010000.serial: deferred probe pending: platform: supplier 10000000.clock-controller not ready
>>>> platform 10011000.serial: deferred probe pending: platform: supplier 10000000.clock-controller not ready
>>>> platform 10040000.spi: deferred probe pending: platform: supplier 10000000.clock-controller not ready
>>>> platform 10050000.spi: deferred probe pending: platform: supplier 10000000.clock-controller not ready
>>>> platform 10090000.ethernet: deferred probe pending: platform: supplier 10000000.clock-controller not ready
>>>>
>>>> when trying to boot sifive_u in qemu.
>>>>
>>>> Apparently, "10000000.clock-controller" is too long. Any suggestion on
>>>> how to solve the problem ? I guess using dev_name(dev) as dev_id parameter
>>>> for clk_hw_register_clkdev() is not or no longer a good idea.
>>>> What else should be used instead ?
>>>
>>> It was *never* a good idea. clkdev uses a fixed buffer size of 20
>>> characters including the NUL character, and "10000000.clock-controller"
>>> would have been silently truncated to "10000000.clock-cont", and thus
>>>
>>>                           if (!dev_id || strcmp(p->dev_id, dev_id))
>>>
>>> would never have matched.
>>>
>>> We need to think about (a) whether your use of clk_hw_register_clkdev()
>>> is still appropriate, and (b) whether we need to increase the size of
>>> the strings.
>>>
>>
>> It isn't _my_ use, really. I only run a variety of boot tests with qemu.
>> I expect we'll see reports from others trying to boot the mainline kernel
>> on real sifive_u hardware or other hardware using the same driver or other
>> drivers using dev_name() as dev_id parameter. Coccinelle finds the
>> following callers:
> 
> Using dev_name() is not an issue. It's when dev_name() exceeds 19
> characters that it becomes an issue (and always has been an issue
> due to the truncation.) clk_get(dev, ...) uses dev_name(dev) to match
> against its entry in the table.
> 
> As I say, dev_name() itself is not an issue. The length used for the
> name is.
> 

Maybe, but the existence of best_dev_name() suggests that this has been seen
before and that, as you mentioned, it is not a good idea. Anyway, the patch
below fixes the problem for me. I don't know if it is acceptable / correct,
so it might serve as guidance for others when fixing the problem for real.

Thanks,
Guenter

---
diff --git a/drivers/clk/sifive/sifive-prci.c b/drivers/clk/sifive/sifive-prci.c
index 25b8e1a80ddc..20cc8f42d9eb 100644
--- a/drivers/clk/sifive/sifive-prci.c
+++ b/drivers/clk/sifive/sifive-prci.c
@@ -537,7 +537,7 @@ static int __prci_register_clocks(struct device *dev, struct __prci_data *pd,
                         return r;
                 }

-               r = clk_hw_register_clkdev(&pic->hw, pic->name, dev_name(dev));
+               r = clk_hw_register_clkdev(&pic->hw, pic->name, "prci");
                 if (r) {
                         dev_warn(dev, "Failed to register clkdev for %s: %d\n",
                                  init.name, r);