* [PATCH v2 1/2] ASoC: cs35l41: Add one more variable in the debug log
@ 2022-03-28 4:22 Hui Wang
2022-03-28 4:22 ` [PATCH v2 2/2] ASoC: cs35l41: Don't hard-code the number of otp_elem in the array Hui Wang
2022-03-28 8:40 ` [PATCH v2 1/2] ASoC: cs35l41: Add one more variable in the debug log Lucas tanure
0 siblings, 2 replies; 6+ messages in thread
From: Hui Wang @ 2022-03-28 4:22 UTC (permalink / raw)
To: alsa-devel, broonie, patches, ckeepax, tanureal
otp_map[].size is a key variable to compute the value of otp_val and
to update the bit_offset, it is helpful to debug if could put it in
the debug log.
Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
sound/soc/codecs/cs35l41-lib.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c
index e5a56bcbb223..d0a480c40231 100644
--- a/sound/soc/codecs/cs35l41-lib.c
+++ b/sound/soc/codecs/cs35l41-lib.c
@@ -822,8 +822,8 @@ int cs35l41_otp_unpack(struct device *dev, struct regmap *regmap)
word_offset = otp_map_match->word_offset;
for (i = 0; i < otp_map_match->num_elements; i++) {
- dev_dbg(dev, "bitoffset= %d, word_offset=%d, bit_sum mod 32=%d\n",
- bit_offset, word_offset, bit_sum % 32);
+ dev_dbg(dev, "bitoffset= %d, word_offset=%d, bit_sum mod 32=%d otp_map[i].size = %d\n",
+ bit_offset, word_offset, bit_sum % 32, otp_map[i].size);
if (bit_offset + otp_map[i].size - 1 >= 32) {
otp_val = (otp_mem[word_offset] &
GENMASK(31, bit_offset)) >> bit_offset;
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] ASoC: cs35l41: Don't hard-code the number of otp_elem in the array
2022-03-28 4:22 [PATCH v2 1/2] ASoC: cs35l41: Add one more variable in the debug log Hui Wang
@ 2022-03-28 4:22 ` Hui Wang
2022-03-28 8:56 ` Lucas tanure
2022-03-28 10:17 ` Charles Keepax
2022-03-28 8:40 ` [PATCH v2 1/2] ASoC: cs35l41: Add one more variable in the debug log Lucas tanure
1 sibling, 2 replies; 6+ messages in thread
From: Hui Wang @ 2022-03-28 4:22 UTC (permalink / raw)
To: alsa-devel, broonie, patches, ckeepax, tanureal
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(-)
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,
},
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] ASoC: cs35l41: Add one more variable in the debug log
2022-03-28 4:22 [PATCH v2 1/2] ASoC: cs35l41: Add one more variable in the debug log Hui Wang
2022-03-28 4:22 ` [PATCH v2 2/2] ASoC: cs35l41: Don't hard-code the number of otp_elem in the array Hui Wang
@ 2022-03-28 8:40 ` Lucas tanure
1 sibling, 0 replies; 6+ messages in thread
From: Lucas tanure @ 2022-03-28 8:40 UTC (permalink / raw)
To: Hui Wang, alsa-devel, broonie, patches, ckeepax
On 3/28/22 05:22, Hui Wang wrote:
> otp_map[].size is a key variable to compute the value of otp_val and
> to update the bit_offset, it is helpful to debug if could put it in
> the debug log.
>
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
> sound/soc/codecs/cs35l41-lib.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c
> index e5a56bcbb223..d0a480c40231 100644
> --- a/sound/soc/codecs/cs35l41-lib.c
> +++ b/sound/soc/codecs/cs35l41-lib.c
> @@ -822,8 +822,8 @@ int cs35l41_otp_unpack(struct device *dev, struct regmap *regmap)
> word_offset = otp_map_match->word_offset;
>
> for (i = 0; i < otp_map_match->num_elements; i++) {
> - dev_dbg(dev, "bitoffset= %d, word_offset=%d, bit_sum mod 32=%d\n",
> - bit_offset, word_offset, bit_sum % 32);
> + dev_dbg(dev, "bitoffset= %d, word_offset=%d, bit_sum mod 32=%d otp_map[i].size = %d\n",
> + bit_offset, word_offset, bit_sum % 32, otp_map[i].size);
otp_map[i].size is u8, please use %u to print it.
And add an "," between "bit_sum mod" and "otp_map[i].size" to keep
consistency.
> if (bit_offset + otp_map[i].size - 1 >= 32) {
> otp_val = (otp_mem[word_offset] &
> GENMASK(31, bit_offset)) >> bit_offset;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] ASoC: cs35l41: Don't hard-code the number of otp_elem in the array
2022-03-28 4:22 ` [PATCH v2 2/2] ASoC: cs35l41: Don't hard-code the number of otp_elem in the array Hui Wang
@ 2022-03-28 8:56 ` Lucas tanure
2022-03-28 11:09 ` Hui Wang
2022-03-28 10:17 ` Charles Keepax
1 sibling, 1 reply; 6+ messages in thread
From: Lucas tanure @ 2022-03-28 8:56 UTC (permalink / raw)
To: Hui Wang, alsa-devel, broonie, patches, ckeepax
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,
> },
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] ASoC: cs35l41: Don't hard-code the number of otp_elem in the array
2022-03-28 4:22 ` [PATCH v2 2/2] ASoC: cs35l41: Don't hard-code the number of otp_elem in the array Hui Wang
2022-03-28 8:56 ` Lucas tanure
@ 2022-03-28 10:17 ` Charles Keepax
1 sibling, 0 replies; 6+ messages in thread
From: Charles Keepax @ 2022-03-28 10:17 UTC (permalink / raw)
To: Hui Wang; +Cc: patches, alsa-devel, broonie, tanureal
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] ASoC: cs35l41: Don't hard-code the number of otp_elem in the array
2022-03-28 8:56 ` Lucas tanure
@ 2022-03-28 11:09 ` Hui Wang
0 siblings, 0 replies; 6+ messages in thread
From: Hui Wang @ 2022-03-28 11:09 UTC (permalink / raw)
To: Lucas tanure, alsa-devel, broonie, patches, ckeepax
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-28 11:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-28 4:22 [PATCH v2 1/2] ASoC: cs35l41: Add one more variable in the debug log Hui Wang
2022-03-28 4:22 ` [PATCH v2 2/2] ASoC: cs35l41: Don't hard-code the number of otp_elem in the array Hui Wang
2022-03-28 8:56 ` Lucas tanure
2022-03-28 11:09 ` Hui Wang
2022-03-28 10:17 ` Charles Keepax
2022-03-28 8:40 ` [PATCH v2 1/2] ASoC: cs35l41: Add one more variable in the debug log Lucas tanure
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.