* [PATCH 1/2] ASoC: cs35l41: Add one more variable in the debug log
@ 2022-03-24 8:18 Hui Wang
2022-03-24 8:18 ` [PATCH 2/2] ASoC: cs35l41: Fix a shift-out-of-bounds warning found by UBSAN Hui Wang
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Hui Wang @ 2022-03-24 8:18 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 2/2] ASoC: cs35l41: Fix a shift-out-of-bounds warning found by UBSAN
2022-03-24 8:18 [PATCH 1/2] ASoC: cs35l41: Add one more variable in the debug log Hui Wang
@ 2022-03-24 8:18 ` Hui Wang
2022-03-25 16:46 ` Lucas tanure
2022-03-25 16:44 ` [PATCH 1/2] ASoC: cs35l41: Add one more variable in the debug log Lucas tanure
2022-04-05 9:31 ` Mark Brown
2 siblings, 1 reply; 6+ messages in thread
From: Hui Wang @ 2022-03-24 8:18 UTC (permalink / raw)
To: alsa-devel, broonie, patches, ckeepax, tanureal
We enabled UBSAN in the ubuntu kernel, and the cs35l41 driver triggers
a warning calltrace like below:
cs35l41-hda i2c-CSC3551:00-cs35l41-hda.0: bitoffset= 8, word_offset=23, bit_sum mod 32=0, otp_map[i].size = 24
cs35l41-hda i2c-CSC3551:00-cs35l41-hda.0: bitoffset= 0, word_offset=24, bit_sum mod 32=24, otp_map[i].size = 0
================================================================================
UBSAN: shift-out-of-bounds in linux-kernel-src/sound/soc/codecs/cs35l41-lib.c:836:8
shift exponent 64 is too large for 64-bit type 'long unsigned int'
CPU: 10 PID: 595 Comm: systemd-udevd Not tainted 5.15.0-23-generic #23
Hardware name: LENOVO \x02MFG_IN_GO/\x02MFG_IN_GO, BIOS N3GET19W (1.00 ) 03/11/2022
Call Trace:
<TASK>
show_stack+0x52/0x58
dump_stack_lvl+0x4a/0x5f
dump_stack+0x10/0x12
ubsan_epilogue+0x9/0x45
__ubsan_handle_shift_out_of_bounds.cold+0x61/0xef
? regmap_unlock_mutex+0xe/0x10
cs35l41_otp_unpack.cold+0x1c6/0x2b2 [snd_soc_cs35l41_lib]
cs35l41_hda_probe+0x24f/0x33a [snd_hda_scodec_cs35l41]
cs35l41_hda_i2c_probe+0x65/0x90 [snd_hda_scodec_cs35l41_i2c]
When both bitoffset and otp_map[i].size are 0, the line 836 will
result in GENMASK(-1, 0), this triggers the shift-out-of-bounds
calltrace.
Here add a checking, if both bitoffset and otp_map[i].size are 0,
do not run GENMASK() and directly set otp_val to 0, this will not
bring any function change on the driver but could avoid the calltrace.
Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
sound/soc/codecs/cs35l41-lib.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c
index d0a480c40231..aa6823fbd1a4 100644
--- a/sound/soc/codecs/cs35l41-lib.c
+++ b/sound/soc/codecs/cs35l41-lib.c
@@ -831,12 +831,14 @@ int cs35l41_otp_unpack(struct device *dev, struct regmap *regmap)
GENMASK(bit_offset + otp_map[i].size - 33, 0)) <<
(32 - bit_offset);
bit_offset += otp_map[i].size - 32;
- } else {
+ } else if (bit_offset + otp_map[i].size - 1 >= 0) {
otp_val = (otp_mem[word_offset] &
GENMASK(bit_offset + otp_map[i].size - 1, bit_offset)
) >> bit_offset;
bit_offset += otp_map[i].size;
- }
+ } else /* both bit_offset and otp_map[i].size are 0 */
+ otp_val = 0;
+
bit_sum += otp_map[i].size;
if (bit_offset == 32) {
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ASoC: cs35l41: Add one more variable in the debug log
2022-03-24 8:18 [PATCH 1/2] ASoC: cs35l41: Add one more variable in the debug log Hui Wang
2022-03-24 8:18 ` [PATCH 2/2] ASoC: cs35l41: Fix a shift-out-of-bounds warning found by UBSAN Hui Wang
@ 2022-03-25 16:44 ` Lucas tanure
2022-04-05 9:31 ` Mark Brown
2 siblings, 0 replies; 6+ messages in thread
From: Lucas tanure @ 2022-03-25 16:44 UTC (permalink / raw)
To: Hui Wang, alsa-devel, broonie, patches, ckeepax
On 3/24/22 08:18, 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>
Reviewed-by: Lucas Tanure <tanureal@opensource.cirrus.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;
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ASoC: cs35l41: Fix a shift-out-of-bounds warning found by UBSAN
2022-03-24 8:18 ` [PATCH 2/2] ASoC: cs35l41: Fix a shift-out-of-bounds warning found by UBSAN Hui Wang
@ 2022-03-25 16:46 ` Lucas tanure
2022-03-28 3:52 ` Hui Wang
0 siblings, 1 reply; 6+ messages in thread
From: Lucas tanure @ 2022-03-25 16:46 UTC (permalink / raw)
To: Hui Wang, alsa-devel, broonie, patches, ckeepax
On 3/24/22 08:18, Hui Wang wrote:
> We enabled UBSAN in the ubuntu kernel, and the cs35l41 driver triggers
> a warning calltrace like below:
>
> cs35l41-hda i2c-CSC3551:00-cs35l41-hda.0: bitoffset= 8, word_offset=23, bit_sum mod 32=0, otp_map[i].size = 24
> cs35l41-hda i2c-CSC3551:00-cs35l41-hda.0: bitoffset= 0, word_offset=24, bit_sum mod 32=24, otp_map[i].size = 0
> ================================================================================
> UBSAN: shift-out-of-bounds in linux-kernel-src/sound/soc/codecs/cs35l41-lib.c:836:8
> shift exponent 64 is too large for 64-bit type 'long unsigned int'
> CPU: 10 PID: 595 Comm: systemd-udevd Not tainted 5.15.0-23-generic #23
> Hardware name: LENOVO \x02MFG_IN_GO/\x02MFG_IN_GO, BIOS N3GET19W (1.00 ) 03/11/2022
> Call Trace:
> <TASK>
> show_stack+0x52/0x58
> dump_stack_lvl+0x4a/0x5f
> dump_stack+0x10/0x12
> ubsan_epilogue+0x9/0x45
> __ubsan_handle_shift_out_of_bounds.cold+0x61/0xef
> ? regmap_unlock_mutex+0xe/0x10
> cs35l41_otp_unpack.cold+0x1c6/0x2b2 [snd_soc_cs35l41_lib]
> cs35l41_hda_probe+0x24f/0x33a [snd_hda_scodec_cs35l41]
> cs35l41_hda_i2c_probe+0x65/0x90 [snd_hda_scodec_cs35l41_i2c]
>
> When both bitoffset and otp_map[i].size are 0, the line 836 will
> result in GENMASK(-1, 0), this triggers the shift-out-of-bounds
> calltrace.
>
> Here add a checking, if both bitoffset and otp_map[i].size are 0,
> do not run GENMASK() and directly set otp_val to 0, this will not
> bring any function change on the driver but could avoid the calltrace.
Here would be better to return an error and fail the probe, as this is
a not expected case.
>
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
> sound/soc/codecs/cs35l41-lib.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/codecs/cs35l41-lib.c b/sound/soc/codecs/cs35l41-lib.c
> index d0a480c40231..aa6823fbd1a4 100644
> --- a/sound/soc/codecs/cs35l41-lib.c
> +++ b/sound/soc/codecs/cs35l41-lib.c
> @@ -831,12 +831,14 @@ int cs35l41_otp_unpack(struct device *dev, struct regmap *regmap)
> GENMASK(bit_offset + otp_map[i].size - 33, 0)) <<
> (32 - bit_offset);
> bit_offset += otp_map[i].size - 32;
> - } else {
> + } else if (bit_offset + otp_map[i].size - 1 >= 0) {
> otp_val = (otp_mem[word_offset] &
> GENMASK(bit_offset + otp_map[i].size - 1, bit_offset)
> ) >> bit_offset;
> bit_offset += otp_map[i].size;
> - }
> + } else /* both bit_offset and otp_map[i].size are 0 */
> + otp_val = 0;
> +
> bit_sum += otp_map[i].size;
>
> if (bit_offset == 32) {
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ASoC: cs35l41: Fix a shift-out-of-bounds warning found by UBSAN
2022-03-25 16:46 ` Lucas tanure
@ 2022-03-28 3:52 ` Hui Wang
0 siblings, 0 replies; 6+ messages in thread
From: Hui Wang @ 2022-03-28 3:52 UTC (permalink / raw)
To: Lucas tanure, alsa-devel, broonie, patches, ckeepax
On 3/26/22 00:46, Lucas tanure wrote:
> On 3/24/22 08:18, Hui Wang wrote:
>> We enabled UBSAN in the ubuntu kernel, and the cs35l41 driver triggers
>> a warning calltrace like below:
>>
>> cs35l41-hda i2c-CSC3551:00-cs35l41-hda.0: bitoffset= 8,
>> word_offset=23, bit_sum mod 32=0, otp_map[i].size = 24
>> cs35l41-hda i2c-CSC3551:00-cs35l41-hda.0: bitoffset= 0,
>> word_offset=24, bit_sum mod 32=24, otp_map[i].size = 0
>> ================================================================================
>>
>> UBSAN: shift-out-of-bounds in
>> linux-kernel-src/sound/soc/codecs/cs35l41-lib.c:836:8
>> shift exponent 64 is too large for 64-bit type 'long unsigned int'
>> CPU: 10 PID: 595 Comm: systemd-udevd Not tainted 5.15.0-23-generic #23
>> Hardware name: LENOVO \x02MFG_IN_GO/\x02MFG_IN_GO, BIOS N3GET19W
>> (1.00 ) 03/11/2022
>> Call Trace:
>> <TASK>
>> show_stack+0x52/0x58
>> dump_stack_lvl+0x4a/0x5f
>> dump_stack+0x10/0x12
>> ubsan_epilogue+0x9/0x45
>> __ubsan_handle_shift_out_of_bounds.cold+0x61/0xef
>> ? regmap_unlock_mutex+0xe/0x10
>> cs35l41_otp_unpack.cold+0x1c6/0x2b2 [snd_soc_cs35l41_lib]
>> cs35l41_hda_probe+0x24f/0x33a [snd_hda_scodec_cs35l41]
>> cs35l41_hda_i2c_probe+0x65/0x90 [snd_hda_scodec_cs35l41_i2c]
>>
>> When both bitoffset and otp_map[i].size are 0, the line 836 will
>> result in GENMASK(-1, 0), this triggers the shift-out-of-bounds
>> calltrace.
>>
>> Here add a checking, if both bitoffset and otp_map[i].size are 0,
>> do not run GENMASK() and directly set otp_val to 0, this will not
>> bring any function change on the driver but could avoid the calltrace.
> Here would be better to return an error and fail the probe, as this is
> a not expected case.
>
OK, got it. And I re-read the code and found It is the last entry in the
array to introduce this call-trace, the CS35L41_NUM_OTP_ELEM is defined
to be 100, but otp_map_1/2[] only defines 99 elements, so the last entry
in the array are {0, 0, 0}, that is why the driver gets a 0 for map[i].size.
I will remove the CS35L41_NUM_OTP_ELEM and use ARRAY_SIZE() in the v2 patch.
Thanks,
Hui.
>>
>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>> ---
>> sound/soc/codecs/cs35l41-lib.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/sound/soc/codecs/cs35l41-lib.c
>> b/sound/soc/codecs/cs35l41-lib.c
>> index d0a480c40231..aa6823fbd1a4 100644
>> --- a/sound/soc/codecs/cs35l41-lib.c
>> +++ b/sound/soc/codecs/cs35l41-lib.c
>> @@ -831,12 +831,14 @@ int cs35l41_otp_unpack(struct device *dev,
>> struct regmap *regmap)
>> GENMASK(bit_offset + otp_map[i].size - 33, 0)) <<
>> (32 - bit_offset);
>> bit_offset += otp_map[i].size - 32;
>> - } else {
>> + } else if (bit_offset + otp_map[i].size - 1 >= 0) {
>> otp_val = (otp_mem[word_offset] &
>> GENMASK(bit_offset + otp_map[i].size - 1,
>> bit_offset)
>> ) >> bit_offset;
>> bit_offset += otp_map[i].size;
>> - }
>> + } else /* both bit_offset and otp_map[i].size are 0 */
>> + otp_val = 0;
>> +
>> bit_sum += otp_map[i].size;
>> if (bit_offset == 32) {
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] ASoC: cs35l41: Add one more variable in the debug log
2022-03-24 8:18 [PATCH 1/2] ASoC: cs35l41: Add one more variable in the debug log Hui Wang
2022-03-24 8:18 ` [PATCH 2/2] ASoC: cs35l41: Fix a shift-out-of-bounds warning found by UBSAN Hui Wang
2022-03-25 16:44 ` [PATCH 1/2] ASoC: cs35l41: Add one more variable in the debug log Lucas tanure
@ 2022-04-05 9:31 ` Mark Brown
2 siblings, 0 replies; 6+ messages in thread
From: Mark Brown @ 2022-04-05 9:31 UTC (permalink / raw)
To: patches, tanureal, alsa-devel, ckeepax, hui.wang
On Thu, 24 Mar 2022 16:18:38 +0800, 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.
>
>
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/2] ASoC: cs35l41: Add one more variable in the debug log
commit: c598ccfbeb26cb9452f99e7beb92ef779dcb16b1
[2/2] ASoC: cs35l41: Fix a shift-out-of-bounds warning found by UBSAN
commit: 0b3d5d2e358ca6772fc3662fca27acb12a682fbf
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-04-05 9:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-24 8:18 [PATCH 1/2] ASoC: cs35l41: Add one more variable in the debug log Hui Wang
2022-03-24 8:18 ` [PATCH 2/2] ASoC: cs35l41: Fix a shift-out-of-bounds warning found by UBSAN Hui Wang
2022-03-25 16:46 ` Lucas tanure
2022-03-28 3:52 ` Hui Wang
2022-03-25 16:44 ` [PATCH 1/2] ASoC: cs35l41: Add one more variable in the debug log Lucas tanure
2022-04-05 9:31 ` Mark Brown
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.