[PATCH v2 1/2] ASoC: cs35l41: Add one more variable in the debug log
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;
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, },
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),
.num_elements = CS35L41_NUM_OTP_ELEM,
.num_elements = ARRAY_SIZE(otp_map_2),
.num_elements = CS35L41_NUM_OTP_ELEM,
.num_elements = ARRAY_SIZE(otp_map_2),
.num_elements = CS35L41_NUM_OTP_ELEM,
.num_elements = ARRAY_SIZE(otp_map_2),
.num_elements = CS35L41_NUM_OTP_ELEM,
.num_elements = ARRAY_SIZE(otp_map_1),
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.
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
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;
participants (3)
-
Charles Keepax -
Hui Wang -
Lucas tanure