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

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

[Samuel Holland]

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.

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] rtc: sun6i: Drop the unused has_out_clk flag

[Samuel Holland]

This flag was never used by the driver.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/rtc/rtc-sun6i.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
index a22358a44e32..52049f139474 100644
--- a/drivers/rtc/rtc-sun6i.c
+++ b/drivers/rtc/rtc-sun6i.c
@@ -135,7 +135,6 @@ struct sun6i_rtc_clk_data {
 	unsigned long rc_osc_rate;
 	unsigned int fixed_prescaler : 16;
 	unsigned int has_prescaler : 1;
-	unsigned int has_out_clk : 1;
 	unsigned int export_iosc : 1;
 	unsigned int has_losc_en : 1;
 	unsigned int has_auto_swt : 1;
@@ -346,7 +345,6 @@ CLK_OF_DECLARE_DRIVER(sun6i_a31_rtc_clk, "allwinner,sun6i-a31-rtc",
 static const struct sun6i_rtc_clk_data sun8i_a23_rtc_data = {
 	.rc_osc_rate = 667000, /* datasheet says 600 ~ 700 KHz */
 	.has_prescaler = 1,
-	.has_out_clk = 1,
 };
 
 static void __init sun8i_a23_rtc_clk_init(struct device_node *node)
@@ -360,7 +358,6 @@ static const struct sun6i_rtc_clk_data sun8i_h3_rtc_data = {
 	.rc_osc_rate = 16000000,
 	.fixed_prescaler = 32,
 	.has_prescaler = 1,
-	.has_out_clk = 1,
 	.export_iosc = 1,
 };
 
@@ -378,7 +375,6 @@ static const struct sun6i_rtc_clk_data sun50i_h6_rtc_data = {
 	.rc_osc_rate = 16000000,
 	.fixed_prescaler = 32,
 	.has_prescaler = 1,
-	.has_out_clk = 1,
 	.export_iosc = 1,
 	.has_losc_en = 1,
 	.has_auto_swt = 1,
@@ -409,7 +405,6 @@ CLK_OF_DECLARE_DRIVER(sun8i_r40_rtc_clk, "allwinner,sun8i-r40-rtc",
 
 static const struct sun6i_rtc_clk_data sun8i_v3_rtc_data = {
 	.rc_osc_rate = 32000,
-	.has_out_clk = 1,
 };
 
 static void __init sun8i_v3_rtc_clk_init(struct device_node *node)
-- 
2.37.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] rtc: sun6i: Drop the unused has_out_clk flag

[Jernej Škrabec]

Dne četrtek, 29. december 2022 ob 19:40:11 CET je Samuel Holland napisal(a):
> This flag was never used by the driver.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jernej Škrabec]

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.

Best regards,
Jernej

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





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Samuel Holland]

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.

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.

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Jernej Škrabec]

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





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Samuel Holland]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Alexandre Belloni]

Hello,

What should I do with this series, I'm not sure you came to an
agreement.
Also, 2/2 doesn't apply so you'd have to rebase.

On 29/12/2022 12:40:10-0600, Samuel Holland wrote:
> 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.
> 
> 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);
>  
> -- 
> 2.37.4
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Samuel Holland]

On 2/9/23 16:49, Alexandre Belloni wrote:
> Hello,
> 
> What should I do with this series, I'm not sure you came to an
> agreement.
> Also, 2/2 doesn't apply so you'd have to rebase.

I will send v2 after the merge window, possibly including only patch 2.

Regards,
Samuel

> On 29/12/2022 12:40:10-0600, Samuel Holland wrote:
>> 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.
>>
>> 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);
>>  
>> -- 
>> 2.37.4
>>
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel