Feedback-ID: i0ad843c9:Fastmail
Message-ID: <90760cb1-81b2-ad83-0c62-f1c8180b0c0f@sholland.org>
Date:   Sun, 12 Feb 2023 15:10:00 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux ppc64le; rv:102.0) Gecko/20100101
 Thunderbird/102.3.3
Content-Language: en-US
To:     =?UTF-8?Q?Jernej_=c5=a0krabec?= <jernej.skrabec@gmail.com>
Cc:     linux-arm-kernel@lists.infradead.org, linux-rtc@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-sunxi@lists.linux.dev,
        Alessandro Zummo <a.zummo@towertech.it>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Chen-Yu Tsai <wens@csie.org>
References: <20221229184011.62925-1-samuel@sholland.org>
 <8201852.NyiUUSuA9g@jernej-laptop>
 <5c20af59-5fb5-8f7d-f6af-2b3984d79595@sholland.org>
 <4834080.31r3eYUQgx@jernej-laptop>
From:   Samuel Holland <samuel@sholland.org>
Subject: Re: [PATCH 1/2] rtc: sun6i: Prevent an out-of-bounds read
In-Reply-To: <4834080.31r3eYUQgx@jernej-laptop>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@sholland.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);