linux-kernel - Re: [PATCH 1/2] rtc: sun6i: Prevent an out-of-bounds read

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: <90760cb1-81b2-ad83-0c62-f1c8180b0c0f@sholland.org>
Date:   Sun, 12 Feb 2023 15:10:00 -0600
From:   Samuel Holland <samuel@...lland.org>
To:     Jernej Škrabec <jernej.skrabec@...il.com>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-rtc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-sunxi@...ts.linux.dev,
        Alessandro Zummo <a.zummo@...ertech.it>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Chen-Yu Tsai <wens@...e.org>
Subject: Re: [PATCH 1/2] rtc: sun6i: Prevent an out-of-bounds read

Hi Jernej,

On 1/8/23 13:39, Jernej Škrabec wrote:
> Dne sobota, 07. januar 2023 ob 18:15:47 CET je Samuel Holland napisal(a):
>> On 1/5/23 11:26, Jernej Škrabec wrote:
>>> Dne četrtek, 29. december 2022 ob 19:40:10 CET je Samuel Holland napisal(a):
>>>> If there is more than one parent clock in the devicetree, the
>>>> driver sets .num_parents to a larger value than the number of array
>>>> elements, which causes an out-of-bounds read in the clock framework.
>>>
>>> Is there any DT with more than one parent? I think more fixes are needed
>>> if
>>> this is the case.
>>
>> H616 and newer expect more than one parent, to accurately represent the
>> RTC clock tree, but they use the CCU driver instead of this code.
> 
> If I understand that correctly, second clock would be 24 MHz crystal? In any 

That is correct.

> case, if multiple parents are possible, check needs to be added to see if 
> parent clocks include 32 kHz clock or not.

Right, if we allow other clock inputs, we need to check specifically for
"ext-osc32k", or a single clock input without clock-names, not just the
presence of the clocks property. (A hypothetical new binding would have
to require clock-names even for a single clock to distinguish the old
binding with only "ext-osc32k" from the new binding with only "hosc".)

>> This bug is preventing us from relaxing `maxItems` in the binding for H6
>> and older SoCs, even if Linux does not use the additional parent clocks.
>> I want to fix this bug now, to give us the option (if beneficial) of
>> relaxing the binding in the long-term future.
> 
> I wouldn't call it a bug, since it works just fine for currently defined 
> binding. Do you have DT binding change in pipeline?

This would be a far future change, so as to not break the "old kernel +
new DT" scenario. Maybe it's not even worth doing. But I really don't
like the unbounded assignment to num_parents here.

Regards,
Samuel

>>>> Fix this by coercing the parent count to a Boolean value, like the
>>>> driver expects.
>>>>
>>>> Fixes: 3855c2c3e546 ("rtc: sun6i: Expose the 32kHz oscillator")
>>>> Signed-off-by: Samuel Holland <samuel@...lland.org>
>>>> ---
>>>>
>>>>  drivers/rtc/rtc-sun6i.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
>>>> index ed5516089e9a..a22358a44e32 100644
>>>> --- a/drivers/rtc/rtc-sun6i.c
>>>> +++ b/drivers/rtc/rtc-sun6i.c
>>>> @@ -294,7 +294,7 @@ static void __init sun6i_rtc_clk_init(struct
>>>> device_node *node,
>>>>
>>>>  	init.parent_names = parents;
>>>>  	/* ... number of clock parents will be 1. */
>>>>
>>>> -	init.num_parents = of_clk_get_parent_count(node) + 1;
>>>> +	init.num_parents = !!of_clk_get_parent_count(node) + 1;
>>>>
>>>>  	of_property_read_string_index(node, "clock-output-names", 0,
>>>>  	
>>>>  				      &init.name);