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: <4834080.31r3eYUQgx@jernej-laptop>
Date:   Sun, 08 Jan 2023 20:39:27 +0100
From:   Jernej Škrabec <jernej.skrabec@...il.com>
To:     Alessandro Zummo <a.zummo@...ertech.it>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Chen-Yu Tsai <wens@...e.org>,
        Samuel Holland <samuel@...lland.org>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-rtc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-sunxi@...ts.linux.dev
Subject: Re: [PATCH 1/2] rtc: sun6i: Prevent an out-of-bounds read

Dne sobota, 07. januar 2023 ob 18:15:47 CET je Samuel Holland napisal(a):
> Hi Jernej,
> 
> 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 
case, if multiple parents are possible, check needs to be added to see if 
parent clocks include 32 kHz clock or not.

> 
> 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?

Best regards,
Jernej

> 
> 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);