| Message ID | 20220324081839.62009-1-hui.wang@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] ASoC: cs35l41: Add one more variable in the debug log | expand |
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;
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) {
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) { >
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;
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(-)