diff mbox series

[v2,2/2] ASoC: cs35l41: Don't hard-code the number of otp_elem in the array

Message ID 20220328042210.37660-2-hui.wang@canonical.com (mailing list archive)
State Superseded
Headers show
Series [v2,1/2] ASoC: cs35l41: Add one more variable in the debug log | expand

Commit Message

Hui Wang March 28, 2022, 4:22 a.m. UTC
The CS35L41_NUM_OTP_ELEM is 100, but only 99 entries are defined in
the array otp_map_1/2[CS35L41_NUM_OTP_ELEM], this will trigger UBSAN
to report a shift-out-of-bounds warning in the cs35l41_otp_unpack()
since the last entry in the array will resuilt in GENMASK(-1, 0).

To fix it, removing the definition CS35L41_NUM_OTP_ELEM and use
ARRAY_SIZE to calculate the number of elements dynamically.

Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
 include/sound/cs35l41.h        |  1 -
 sound/soc/codecs/cs35l41-lib.c | 14 +++++++-------
 2 files changed, 7 insertions(+), 8 deletions(-)

Comments

Lucas Tanure March 28, 2022, 8:56 a.m. UTC | #1
On 3/28/22 05:22, Hui Wang wrote:
> The CS35L41_NUM_OTP_ELEM is 100, but only 99 entries are defined in
> the array otp_map_1/2[CS35L41_NUM_OTP_ELEM], this will trigger UBSAN
> to report a shift-out-of-bounds warning in the cs35l41_otp_unpack()
> since the last entry in the array will resuilt in GENMASK(-1, 0).
result
> 
> To fix it, removing the definition CS35L41_NUM_OTP_ELEM and use
> ARRAY_SIZE to calculate the number of elements dynamically.
This a plain out-of-bounds access issue, you could just say that.
And at the end, you could say that UBSAN reported the issue.

Also the title should start with Fix, like:
"Fix out-of-bounds access in cs35l41_otp_packed_element_t"

> 
Fixes: 6450ef559056 ("ASoC: cs35l41: CS35L41 Boosted Smart Amplifier")
> Signed-off-by: Hui Wang <hui.wang@canonical.com>

You are missing the Fixes tag.


> ---
>   include/sound/cs35l41.h        |  1 -
>   sound/soc/codecs/cs35l41-lib.c | 14 +++++++-------
>   2 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/include/sound/cs35l41.h b/include/sound/cs35l41.h
> index bf7f9a9aeba0..9341130257ea 100644
> --- a/include/sound/cs35l41.h
> +++ b/include/sound/cs35l41.h
> @@ -536,7 +536,6 @@
>   
>   #define CS35L41_MAX_CACHE_REG		36
>   #define CS35L41_OTP_SIZE_WORDS		32
> -#define CS35L41_NUM_OTP_ELEM		100
>   
>   #define CS35L41_VALID_PDATA		0x80000000
>   #define CS35L41_NUM_SUPPLIES            2
> diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c
> index d0a480c40231..30c720af98d0 100644
> --- a/sound/soc/codecs/cs35l41-lib.c
> +++ b/sound/soc/codecs/cs35l41-lib.c
> @@ -422,7 +422,7 @@ static bool cs35l41_volatile_reg(struct device *dev, unsigned int reg)
>   	}
>   }
>   
> -static const struct cs35l41_otp_packed_element_t otp_map_1[CS35L41_NUM_OTP_ELEM] = {
> +static const struct cs35l41_otp_packed_element_t otp_map_1[] = {
>   	/* addr         shift   size */
>   	{ 0x00002030,	0,	4 }, /*TRIM_OSC_FREQ_TRIM*/
>   	{ 0x00002030,	7,	1 }, /*TRIM_OSC_TRIM_DONE*/
> @@ -525,7 +525,7 @@ static const struct cs35l41_otp_packed_element_t otp_map_1[CS35L41_NUM_OTP_ELEM]
>   	{ 0x00017044,	0,	24 }, /*LOT_NUMBER*/
>   };
>   
> -static const struct cs35l41_otp_packed_element_t otp_map_2[CS35L41_NUM_OTP_ELEM] = {
> +static const struct cs35l41_otp_packed_element_t otp_map_2[] = {
>   	/* addr         shift   size */
>   	{ 0x00002030,	0,	4 }, /*TRIM_OSC_FREQ_TRIM*/
>   	{ 0x00002030,	7,	1 }, /*TRIM_OSC_TRIM_DONE*/
> @@ -671,35 +671,35 @@ static const struct cs35l41_otp_map_element_t cs35l41_otp_map_map[] = {
>   	{
>   		.id = 0x01,
>   		.map = otp_map_1,
> -		.num_elements = CS35L41_NUM_OTP_ELEM,
> +		.num_elements = ARRAY_SIZE(otp_map_1),
>   		.bit_offset = 16,
>   		.word_offset = 2,
>   	},
>   	{
>   		.id = 0x02,
>   		.map = otp_map_2,
> -		.num_elements = CS35L41_NUM_OTP_ELEM,
> +		.num_elements = ARRAY_SIZE(otp_map_2),
>   		.bit_offset = 16,
>   		.word_offset = 2,
>   	},
>   	{
>   		.id = 0x03,
>   		.map = otp_map_2,
> -		.num_elements = CS35L41_NUM_OTP_ELEM,
> +		.num_elements = ARRAY_SIZE(otp_map_2),
>   		.bit_offset = 16,
>   		.word_offset = 2,
>   	},
>   	{
>   		.id = 0x06,
>   		.map = otp_map_2,
> -		.num_elements = CS35L41_NUM_OTP_ELEM,
> +		.num_elements = ARRAY_SIZE(otp_map_2),
>   		.bit_offset = 16,
>   		.word_offset = 2,
>   	},
>   	{
>   		.id = 0x08,
>   		.map = otp_map_1,
> -		.num_elements = CS35L41_NUM_OTP_ELEM,
> +		.num_elements = ARRAY_SIZE(otp_map_1),
>   		.bit_offset = 16,
>   		.word_offset = 2,
>   	},
Charles Keepax March 28, 2022, 10:17 a.m. UTC | #2
On Mon, Mar 28, 2022 at 12:22:10PM +0800, Hui Wang wrote:
> The CS35L41_NUM_OTP_ELEM is 100, but only 99 entries are defined in
> the array otp_map_1/2[CS35L41_NUM_OTP_ELEM], this will trigger UBSAN
> to report a shift-out-of-bounds warning in the cs35l41_otp_unpack()
> since the last entry in the array will resuilt in GENMASK(-1, 0).
> 
> To fix it, removing the definition CS35L41_NUM_OTP_ELEM and use
> ARRAY_SIZE to calculate the number of elements dynamically.
> 
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---

Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles
Hui Wang March 28, 2022, 11:09 a.m. UTC | #3
On 3/28/22 16:56, Lucas tanure wrote:
> On 3/28/22 05:22, Hui Wang wrote:
>> The CS35L41_NUM_OTP_ELEM is 100, but only 99 entries are defined in
>> the array otp_map_1/2[CS35L41_NUM_OTP_ELEM], this will trigger UBSAN
>> to report a shift-out-of-bounds warning in the cs35l41_otp_unpack()
>> since the last entry in the array will resuilt in GENMASK(-1, 0).
> result
>>
>> To fix it, removing the definition CS35L41_NUM_OTP_ELEM and use
>> ARRAY_SIZE to calculate the number of elements dynamically.
> This a plain out-of-bounds access issue, you could just say that.
> And at the end, you could say that UBSAN reported the issue.
>
> Also the title should start with Fix, like:
> "Fix out-of-bounds access in cs35l41_otp_packed_element_t"
>
>>
> Fixes: 6450ef559056 ("ASoC: cs35l41: CS35L41 Boosted Smart Amplifier")
>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>
> You are missing the Fixes tag.
>
OK, got it, will address all comment.

Thanks.
diff mbox series

Patch

diff --git a/include/sound/cs35l41.h b/include/sound/cs35l41.h
index bf7f9a9aeba0..9341130257ea 100644
--- a/include/sound/cs35l41.h
+++ b/include/sound/cs35l41.h
@@ -536,7 +536,6 @@ 
 
 #define CS35L41_MAX_CACHE_REG		36
 #define CS35L41_OTP_SIZE_WORDS		32
-#define CS35L41_NUM_OTP_ELEM		100
 
 #define CS35L41_VALID_PDATA		0x80000000
 #define CS35L41_NUM_SUPPLIES            2
diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c
index d0a480c40231..30c720af98d0 100644
--- a/sound/soc/codecs/cs35l41-lib.c
+++ b/sound/soc/codecs/cs35l41-lib.c
@@ -422,7 +422,7 @@  static bool cs35l41_volatile_reg(struct device *dev, unsigned int reg)
 	}
 }
 
-static const struct cs35l41_otp_packed_element_t otp_map_1[CS35L41_NUM_OTP_ELEM] = {
+static const struct cs35l41_otp_packed_element_t otp_map_1[] = {
 	/* addr         shift   size */
 	{ 0x00002030,	0,	4 }, /*TRIM_OSC_FREQ_TRIM*/
 	{ 0x00002030,	7,	1 }, /*TRIM_OSC_TRIM_DONE*/
@@ -525,7 +525,7 @@  static const struct cs35l41_otp_packed_element_t otp_map_1[CS35L41_NUM_OTP_ELEM]
 	{ 0x00017044,	0,	24 }, /*LOT_NUMBER*/
 };
 
-static const struct cs35l41_otp_packed_element_t otp_map_2[CS35L41_NUM_OTP_ELEM] = {
+static const struct cs35l41_otp_packed_element_t otp_map_2[] = {
 	/* addr         shift   size */
 	{ 0x00002030,	0,	4 }, /*TRIM_OSC_FREQ_TRIM*/
 	{ 0x00002030,	7,	1 }, /*TRIM_OSC_TRIM_DONE*/
@@ -671,35 +671,35 @@  static const struct cs35l41_otp_map_element_t cs35l41_otp_map_map[] = {
 	{
 		.id = 0x01,
 		.map = otp_map_1,
-		.num_elements = CS35L41_NUM_OTP_ELEM,
+		.num_elements = ARRAY_SIZE(otp_map_1),
 		.bit_offset = 16,
 		.word_offset = 2,
 	},
 	{
 		.id = 0x02,
 		.map = otp_map_2,
-		.num_elements = CS35L41_NUM_OTP_ELEM,
+		.num_elements = ARRAY_SIZE(otp_map_2),
 		.bit_offset = 16,
 		.word_offset = 2,
 	},
 	{
 		.id = 0x03,
 		.map = otp_map_2,
-		.num_elements = CS35L41_NUM_OTP_ELEM,
+		.num_elements = ARRAY_SIZE(otp_map_2),
 		.bit_offset = 16,
 		.word_offset = 2,
 	},
 	{
 		.id = 0x06,
 		.map = otp_map_2,
-		.num_elements = CS35L41_NUM_OTP_ELEM,
+		.num_elements = ARRAY_SIZE(otp_map_2),
 		.bit_offset = 16,
 		.word_offset = 2,
 	},
 	{
 		.id = 0x08,
 		.map = otp_map_1,
-		.num_elements = CS35L41_NUM_OTP_ELEM,
+		.num_elements = ARRAY_SIZE(otp_map_1),
 		.bit_offset = 16,
 		.word_offset = 2,
 	},