[PATCH 0/2] OPP: fix buffer overflow in indexed freq and bandwidth reads

[PATCH 0/2] OPP: fix buffer overflow in indexed freq and bandwidth reads

[Neil Armstrong]

While fixing a crash when calling dev_pm_opp_find_bw_ceil()
because the bandwdith table wasn't initialized, it happens
the index is not checked aswell for indexed freq finds.

In order to properly fix that, pass the index to the assert
function and add a specialized assert function for freq
and bandwidth.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Neil Armstrong (2):
      OPP: add index check to assert to avoid buffer overflow in _read_freq()
      OPP: fix dev_pm_opp_find_bw_*() when bandwidth table not initialized

 drivers/opp/core.c | 52 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 16 deletions(-)
---
base-commit: 6f3d2b5299b0a8bcb8a9405a8d3fceb24f79c4f0
change-id: 20241128-topic-opp-fix-assert-index-check-f46fe5c3c7ba

Best regards,
-- 
Neil Armstrong <neil.armstrong@linaro.org>

[PATCH 1/2] OPP: add index check to assert to avoid buffer overflow in _read_freq()

[Neil Armstrong]

Pass the freq index to the assert function to make sure
we do not read a freq out of the opp->rates[] table.

Without that the index value is never checked when called from
dev_pm_opp_find_freq_exact_indexed() or
dev_pm_opp_find_freq_ceil/floor_indexed().

Add a specialized version of assert_single_clk() to check for
the index to be used with the generic find functions.

Fixes: 142e17c1c2b4 ("OPP: Introduce dev_pm_opp_find_freq_{ceil/floor}_indexed() APIs")
Fixes: a5893928bb17 ("OPP: Add dev_pm_opp_find_freq_exact_indexed()")
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/opp/core.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index d4a0030a0228d6282d672e3ffe3aeea27e80822a..8692e8ce05b7c31a725ea3a7928f238c7a1d6f51 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -106,6 +106,14 @@ static bool assert_single_clk(struct opp_table *opp_table)
 	return !WARN_ON(opp_table->clk_count > 1);
 }
 
+/*
+ * Returns true if clock table is large enough to contain the clock index.
+ */
+static bool assert_clk_index(struct opp_table *opp_table, int index)
+{
+	return opp_table->clk_count > index;
+}
+
 /**
  * dev_pm_opp_get_bw() - Gets the bandwidth corresponding to an opp
  * @opp:	opp for which bandwidth has to be returned for
@@ -524,12 +532,12 @@ static struct dev_pm_opp *_opp_table_find_key(struct opp_table *opp_table,
 		unsigned long (*read)(struct dev_pm_opp *opp, int index),
 		bool (*compare)(struct dev_pm_opp **opp, struct dev_pm_opp *temp_opp,
 				unsigned long opp_key, unsigned long key),
-		bool (*assert)(struct opp_table *opp_table))
+		bool (*assert)(struct opp_table *opp_table, int index))
 {
 	struct dev_pm_opp *temp_opp, *opp = ERR_PTR(-ERANGE);
 
 	/* Assert that the requirement is met */
-	if (assert && !assert(opp_table))
+	if (assert && !assert(opp_table, index))
 		return ERR_PTR(-EINVAL);
 
 	mutex_lock(&opp_table->lock);
@@ -557,7 +565,7 @@ _find_key(struct device *dev, unsigned long *key, int index, bool available,
 	  unsigned long (*read)(struct dev_pm_opp *opp, int index),
 	  bool (*compare)(struct dev_pm_opp **opp, struct dev_pm_opp *temp_opp,
 			  unsigned long opp_key, unsigned long key),
-	  bool (*assert)(struct opp_table *opp_table))
+	  bool (*assert)(struct opp_table *opp_table, int index))
 {
 	struct opp_table *opp_table;
 	struct dev_pm_opp *opp;
@@ -580,7 +588,7 @@ _find_key(struct device *dev, unsigned long *key, int index, bool available,
 static struct dev_pm_opp *_find_key_exact(struct device *dev,
 		unsigned long key, int index, bool available,
 		unsigned long (*read)(struct dev_pm_opp *opp, int index),
-		bool (*assert)(struct opp_table *opp_table))
+		bool (*assert)(struct opp_table *opp_table, int index))
 {
 	/*
 	 * The value of key will be updated here, but will be ignored as the
@@ -593,7 +601,7 @@ static struct dev_pm_opp *_find_key_exact(struct device *dev,
 static struct dev_pm_opp *_opp_table_find_key_ceil(struct opp_table *opp_table,
 		unsigned long *key, int index, bool available,
 		unsigned long (*read)(struct dev_pm_opp *opp, int index),
-		bool (*assert)(struct opp_table *opp_table))
+		bool (*assert)(struct opp_table *opp_table, int index))
 {
 	return _opp_table_find_key(opp_table, key, index, available, read,
 				   _compare_ceil, assert);
@@ -602,7 +610,7 @@ static struct dev_pm_opp *_opp_table_find_key_ceil(struct opp_table *opp_table,
 static struct dev_pm_opp *_find_key_ceil(struct device *dev, unsigned long *key,
 		int index, bool available,
 		unsigned long (*read)(struct dev_pm_opp *opp, int index),
-		bool (*assert)(struct opp_table *opp_table))
+		bool (*assert)(struct opp_table *opp_table, int index))
 {
 	return _find_key(dev, key, index, available, read, _compare_ceil,
 			 assert);
@@ -611,7 +619,7 @@ static struct dev_pm_opp *_find_key_ceil(struct device *dev, unsigned long *key,
 static struct dev_pm_opp *_find_key_floor(struct device *dev,
 		unsigned long *key, int index, bool available,
 		unsigned long (*read)(struct dev_pm_opp *opp, int index),
-		bool (*assert)(struct opp_table *opp_table))
+		bool (*assert)(struct opp_table *opp_table, int index))
 {
 	return _find_key(dev, key, index, available, read, _compare_floor,
 			 assert);
@@ -644,7 +652,7 @@ struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct device *dev,
 		unsigned long freq, bool available)
 {
 	return _find_key_exact(dev, freq, 0, available, _read_freq,
-			       assert_single_clk);
+			       assert_clk_index);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_exact);
 
@@ -672,7 +680,8 @@ struct dev_pm_opp *
 dev_pm_opp_find_freq_exact_indexed(struct device *dev, unsigned long freq,
 				   u32 index, bool available)
 {
-	return _find_key_exact(dev, freq, index, available, _read_freq, NULL);
+	return _find_key_exact(dev, freq, index, available, _read_freq,
+			       assert_clk_index);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_exact_indexed);
 
@@ -680,7 +689,7 @@ static noinline struct dev_pm_opp *_find_freq_ceil(struct opp_table *opp_table,
 						   unsigned long *freq)
 {
 	return _opp_table_find_key_ceil(opp_table, freq, 0, true, _read_freq,
-					assert_single_clk);
+					assert_clk_index);
 }
 
 /**
@@ -704,7 +713,7 @@ static noinline struct dev_pm_opp *_find_freq_ceil(struct opp_table *opp_table,
 struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
 					     unsigned long *freq)
 {
-	return _find_key_ceil(dev, freq, 0, true, _read_freq, assert_single_clk);
+	return _find_key_ceil(dev, freq, 0, true, _read_freq, assert_clk_index);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_ceil);
 
@@ -732,7 +741,8 @@ struct dev_pm_opp *
 dev_pm_opp_find_freq_ceil_indexed(struct device *dev, unsigned long *freq,
 				  u32 index)
 {
-	return _find_key_ceil(dev, freq, index, true, _read_freq, NULL);
+	return _find_key_ceil(dev, freq, index, true, _read_freq,
+			      assert_clk_index);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_ceil_indexed);
 
@@ -757,7 +767,7 @@ EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_ceil_indexed);
 struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
 					      unsigned long *freq)
 {
-	return _find_key_floor(dev, freq, 0, true, _read_freq, assert_single_clk);
+	return _find_key_floor(dev, freq, 0, true, _read_freq, assert_clk_index);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor);
 
@@ -785,7 +795,7 @@ struct dev_pm_opp *
 dev_pm_opp_find_freq_floor_indexed(struct device *dev, unsigned long *freq,
 				   u32 index)
 {
-	return _find_key_floor(dev, freq, index, true, _read_freq, NULL);
+	return _find_key_floor(dev, freq, index, true, _read_freq, assert_clk_index);
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_find_freq_floor_indexed);
 

-- 
2.34.1

Re: [PATCH 1/2] OPP: add index check to assert to avoid buffer overflow in _read_freq()

[Viresh Kumar]

On 28-11-24, 11:07, Neil Armstrong wrote:
> Pass the freq index to the assert function to make sure
> we do not read a freq out of the opp->rates[] table.
> 
> Without that the index value is never checked when called from
> dev_pm_opp_find_freq_exact_indexed() or
> dev_pm_opp_find_freq_ceil/floor_indexed().

These APIs aren't supported for cases where we have more than one clks
available and hence assert for single clk.

-- 
viresh

Re: [PATCH 1/2] OPP: add index check to assert to avoid buffer overflow in _read_freq()

[Neil Armstrong]

Hi,

On 29/11/2024 09:40, Viresh Kumar wrote:
> On 28-11-24, 11:07, Neil Armstrong wrote:
>> Pass the freq index to the assert function to make sure
>> we do not read a freq out of the opp->rates[] table.
>>
>> Without that the index value is never checked when called from
>> dev_pm_opp_find_freq_exact_indexed() or
>> dev_pm_opp_find_freq_ceil/floor_indexed().
> 
> These APIs aren't supported for cases where we have more than one clks
> available and hence assert for single clk.
> 

I don't understand, the _indexed functions clearly have an index parameter
which is documented as "Clock index"

I agree we could leave the other ones with assert_single_clk, but we would
need to duplicate it to have one with the index parameter, so it felt simpler
to use assert_clk_index everywhere but indeed we do not exclude them for
when there's multiple clock...

Neil

Re: [PATCH 1/2] OPP: add index check to assert to avoid buffer overflow in _read_freq()

[Viresh Kumar]

On 29-11-24, 09:53, Neil Armstrong wrote:
> Hi,
> 
> On 29/11/2024 09:40, Viresh Kumar wrote:
> > On 28-11-24, 11:07, Neil Armstrong wrote:
> > > Pass the freq index to the assert function to make sure
> > > we do not read a freq out of the opp->rates[] table.
> > > 
> > > Without that the index value is never checked when called from
> > > dev_pm_opp_find_freq_exact_indexed() or
> > > dev_pm_opp_find_freq_ceil/floor_indexed().
> > 
> > These APIs aren't supported for cases where we have more than one clks
> > available and hence assert for single clk.
> > 
> 
> I don't understand, the _indexed functions clearly have an index parameter
> which is documented as "Clock index"

Ahh, I missed that there are few _indexed() helpers as well which you are
correctly modifying.

> I agree we could leave the other ones with assert_single_clk, but we would
> need to duplicate it to have one with the index parameter, so it felt simpler
> to use assert_clk_index everywhere but indeed we do not exclude them for
> when there's multiple clock...

What prevents a user to call dev_pm_opp_find_freq_exact() for a multi-clk setup
after your patch ? As we use Index = 0 here in the code.

That's why I would prefer the earlier assert for all these, except the indexed
ones.

-- 
viresh

Re: [PATCH 1/2] OPP: add index check to assert to avoid buffer overflow in _read_freq()

[Neil Armstrong]

On 29/11/2024 10:04, Viresh Kumar wrote:
> On 29-11-24, 09:53, Neil Armstrong wrote:
>> Hi,
>>
>> On 29/11/2024 09:40, Viresh Kumar wrote:
>>> On 28-11-24, 11:07, Neil Armstrong wrote:
>>>> Pass the freq index to the assert function to make sure
>>>> we do not read a freq out of the opp->rates[] table.
>>>>
>>>> Without that the index value is never checked when called from
>>>> dev_pm_opp_find_freq_exact_indexed() or
>>>> dev_pm_opp_find_freq_ceil/floor_indexed().
>>>
>>> These APIs aren't supported for cases where we have more than one clks
>>> available and hence assert for single clk.
>>>
>>
>> I don't understand, the _indexed functions clearly have an index parameter
>> which is documented as "Clock index"
> 
> Ahh, I missed that there are few _indexed() helpers as well which you are
> correctly modifying.
> 
>> I agree we could leave the other ones with assert_single_clk, but we would
>> need to duplicate it to have one with the index parameter, so it felt simpler
>> to use assert_clk_index everywhere but indeed we do not exclude them for
>> when there's multiple clock...
> 
> What prevents a user to call dev_pm_opp_find_freq_exact() for a multi-clk setup
> after your patch ? As we use Index = 0 here in the code.
> 
> That's why I would prefer the earlier assert for all these, except the indexed
> ones.

Yes, let me send a v2 with this addressed

Thanks,
Neil

[PATCH 2/2] OPP: fix dev_pm_opp_find_bw_*() when bandwidth table not initialized

[Neil Armstrong]

If a driver calls dev_pm_opp_find_bw_ceil/floor() the retrieve bandwidth
from the OPP table but the bandwidth table was not created because the
interconnect properties were missing in the OPP consumer node, the
kernel will crash with:
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000004
...
pc : _read_bw+0x8/0x10
lr : _opp_table_find_key+0x9c/0x174
...
Call trace:
  _read_bw+0x8/0x10 (P)
  _opp_table_find_key+0x9c/0x174 (L)
  _find_key+0x98/0x168
  dev_pm_opp_find_bw_ceil+0x50/0x88
...

In order to fix the crash, create an assert function to check
if the bandwidth table were created before trying to get a
bandwidth with _read_bw().

Fixes: add1dc094a74 ("OPP: Use generic key finding helpers for bandwidth key")
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/opp/core.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 8692e8ce05b7c31a725ea3a7928f238c7a1d6f51..178780e294dad49c22d866930efb7b8b13ae2d61 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -114,6 +114,14 @@ static bool assert_clk_index(struct opp_table *opp_table, int index)
 	return opp_table->clk_count > index;
 }
 
+/*
+ * Returns true if bandwidth table is large enough to contain the bandwidth index.
+ */
+static bool assert_bandwidth_index(struct opp_table *opp_table, int index)
+{
+	return opp_table->path_count > index;
+}
+
 /**
  * dev_pm_opp_get_bw() - Gets the bandwidth corresponding to an opp
  * @opp:	opp for which bandwidth has to be returned for
@@ -913,7 +921,8 @@ struct dev_pm_opp *dev_pm_opp_find_bw_ceil(struct device *dev, unsigned int *bw,
 	unsigned long temp = *bw;
 	struct dev_pm_opp *opp;
 
-	opp = _find_key_ceil(dev, &temp, index, true, _read_bw, NULL);
+	opp = _find_key_ceil(dev, &temp, index, true, _read_bw,
+			     assert_bandwidth_index);
 	*bw = temp;
 	return opp;
 }
@@ -944,7 +953,8 @@ struct dev_pm_opp *dev_pm_opp_find_bw_floor(struct device *dev,
 	unsigned long temp = *bw;
 	struct dev_pm_opp *opp;
 
-	opp = _find_key_floor(dev, &temp, index, true, _read_bw, NULL);
+	opp = _find_key_floor(dev, &temp, index, true, _read_bw,
+			      assert_bandwidth_index);
 	*bw = temp;
 	return opp;
 }

-- 
2.34.1

Re: [PATCH 2/2] OPP: fix dev_pm_opp_find_bw_*() when bandwidth table not initialized

[Viresh Kumar]

On 28-11-24, 11:07, Neil Armstrong wrote:
> If a driver calls dev_pm_opp_find_bw_ceil/floor() the retrieve bandwidth
> from the OPP table but the bandwidth table was not created because the
> interconnect properties were missing in the OPP consumer node, the
> kernel will crash with:
> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000004
> ...
> pc : _read_bw+0x8/0x10
> lr : _opp_table_find_key+0x9c/0x174
> ...
> Call trace:
>   _read_bw+0x8/0x10 (P)
>   _opp_table_find_key+0x9c/0x174 (L)
>   _find_key+0x98/0x168
>   dev_pm_opp_find_bw_ceil+0x50/0x88
> ...
> 
> In order to fix the crash, create an assert function to check
> if the bandwidth table were created before trying to get a
> bandwidth with _read_bw().
> 
> Fixes: add1dc094a74 ("OPP: Use generic key finding helpers for bandwidth key")
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  drivers/opp/core.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 8692e8ce05b7c31a725ea3a7928f238c7a1d6f51..178780e294dad49c22d866930efb7b8b13ae2d61 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -114,6 +114,14 @@ static bool assert_clk_index(struct opp_table *opp_table, int index)
>  	return opp_table->clk_count > index;
>  }
>  
> +/*
> + * Returns true if bandwidth table is large enough to contain the bandwidth index.
> + */
> +static bool assert_bandwidth_index(struct opp_table *opp_table, int index)
> +{
> +	return opp_table->path_count > index;
> +}
> +
>  /**
>   * dev_pm_opp_get_bw() - Gets the bandwidth corresponding to an opp
>   * @opp:	opp for which bandwidth has to be returned for
> @@ -913,7 +921,8 @@ struct dev_pm_opp *dev_pm_opp_find_bw_ceil(struct device *dev, unsigned int *bw,
>  	unsigned long temp = *bw;
>  	struct dev_pm_opp *opp;
>  
> -	opp = _find_key_ceil(dev, &temp, index, true, _read_bw, NULL);
> +	opp = _find_key_ceil(dev, &temp, index, true, _read_bw,
> +			     assert_bandwidth_index);
>  	*bw = temp;
>  	return opp;
>  }
> @@ -944,7 +953,8 @@ struct dev_pm_opp *dev_pm_opp_find_bw_floor(struct device *dev,
>  	unsigned long temp = *bw;
>  	struct dev_pm_opp *opp;
>  
> -	opp = _find_key_floor(dev, &temp, index, true, _read_bw, NULL);
> +	opp = _find_key_floor(dev, &temp, index, true, _read_bw,
> +			      assert_bandwidth_index);
>  	*bw = temp;
>  	return opp;
>  }

Applied. Thanks.

-- 
viresh

Re: [PATCH 2/2] OPP: fix dev_pm_opp_find_bw_*() when bandwidth table not initialized

[Neil Armstrong]

On 29/11/2024 09:41, Viresh Kumar wrote:
> On 28-11-24, 11:07, Neil Armstrong wrote:
>> If a driver calls dev_pm_opp_find_bw_ceil/floor() the retrieve bandwidth
>> from the OPP table but the bandwidth table was not created because the
>> interconnect properties were missing in the OPP consumer node, the
>> kernel will crash with:
>> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000004
>> ...
>> pc : _read_bw+0x8/0x10
>> lr : _opp_table_find_key+0x9c/0x174
>> ...
>> Call trace:
>>    _read_bw+0x8/0x10 (P)
>>    _opp_table_find_key+0x9c/0x174 (L)
>>    _find_key+0x98/0x168
>>    dev_pm_opp_find_bw_ceil+0x50/0x88
>> ...
>>
>> In order to fix the crash, create an assert function to check
>> if the bandwidth table were created before trying to get a
>> bandwidth with _read_bw().
>>
>> Fixes: add1dc094a74 ("OPP: Use generic key finding helpers for bandwidth key")
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>>   drivers/opp/core.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index 8692e8ce05b7c31a725ea3a7928f238c7a1d6f51..178780e294dad49c22d866930efb7b8b13ae2d61 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -114,6 +114,14 @@ static bool assert_clk_index(struct opp_table *opp_table, int index)
>>   	return opp_table->clk_count > index;
>>   }
>>   
>> +/*
>> + * Returns true if bandwidth table is large enough to contain the bandwidth index.
>> + */
>> +static bool assert_bandwidth_index(struct opp_table *opp_table, int index)
>> +{
>> +	return opp_table->path_count > index;
>> +}
>> +
>>   /**
>>    * dev_pm_opp_get_bw() - Gets the bandwidth corresponding to an opp
>>    * @opp:	opp for which bandwidth has to be returned for
>> @@ -913,7 +921,8 @@ struct dev_pm_opp *dev_pm_opp_find_bw_ceil(struct device *dev, unsigned int *bw,
>>   	unsigned long temp = *bw;
>>   	struct dev_pm_opp *opp;
>>   
>> -	opp = _find_key_ceil(dev, &temp, index, true, _read_bw, NULL);
>> +	opp = _find_key_ceil(dev, &temp, index, true, _read_bw,
>> +			     assert_bandwidth_index);
>>   	*bw = temp;
>>   	return opp;
>>   }
>> @@ -944,7 +953,8 @@ struct dev_pm_opp *dev_pm_opp_find_bw_floor(struct device *dev,
>>   	unsigned long temp = *bw;
>>   	struct dev_pm_opp *opp;
>>   
>> -	opp = _find_key_floor(dev, &temp, index, true, _read_bw, NULL);
>> +	opp = _find_key_floor(dev, &temp, index, true, _read_bw,
>> +			      assert_bandwidth_index);
>>   	*bw = temp;
>>   	return opp;
>>   }
> 
> Applied. Thanks.
> 

Wait, this needs the first patch to work, otherwise index is not passed to assert

Neil

Re: [PATCH 2/2] OPP: fix dev_pm_opp_find_bw_*() when bandwidth table not initialized

[Viresh Kumar]

On 29-11-24, 10:34, Neil Armstrong wrote:
> Wait, this needs the first patch to work, otherwise index is not passed to assert

Oops. My bad.

-- 
viresh