This is a patch that fixes a bug:
KMSAN: uninit-value in dib3000mb_attach (2)
Local variable u8 rb[2] is not initialized as it is used as read buffer
for i2c_transfer(). It is expected that i2c_transfer() should fill in
the buffer before the target function returns rb's content. However
error handling of i2c_transfer is not done, and on occasions where the
read fails, uninitialized rb value will be returned.
The usage of this function, defined as macro rd() in
drivers/media/dvb-frontends/dib3000mb_priv,h, does not expect any error
to occur. Adding error handling here might involve significant code
changes.
Thus 0-initialization is done on rb. This might affect some logic on
error case as the use of the return value is used as boolean and flags.
Reported-by: [email protected]
Link: https://syzkaller.appspot.com/bug?id=2f4d19de8c9e9f0b9794e53ca54d68e0ffe9f068
Signed-off-by: (Ben) HokChun Ng <[email protected]>
---
drivers/media/dvb-frontends/dib3000mb.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/media/dvb-frontends/dib3000mb.c b/drivers/media/dvb-frontends/dib3000mb.c
index a6c2fc4586eb..0dd96656aaf4 100644
--- a/drivers/media/dvb-frontends/dib3000mb.c
+++ b/drivers/media/dvb-frontends/dib3000mb.c
@@ -50,15 +50,19 @@ MODULE_PARM_DESC(debug, "set debugging level (1=info,2=xfer,4=setfe,8=getfe (|-a
static int dib3000_read_reg(struct dib3000_state *state, u16 reg)
{
+ int errno;
u8 wb[] = { ((reg >> 8) | 0x80) & 0xff, reg & 0xff };
- u8 rb[2];
+ u8 rb[2] = { 0, 0 };
struct i2c_msg msg[] = {
{ .addr = state->config.demod_address, .flags = 0, .buf = wb, .len = 2 },
{ .addr = state->config.demod_address, .flags = I2C_M_RD, .buf = rb, .len = 2 },
};
- if (i2c_transfer(state->i2c, msg, 2) != 2)
- deb_i2c("i2c read error\n");
+ errno = i2c_transfer(state->i2c, msg, 2);
+ if (errno != 2) {
+ deb_i2c("i2c read error (errno: %d)\n", -errno);
+ return 0;
+ }
deb_i2c("reading i2c bus (reg: %5d 0x%04x, val: %5d 0x%04x)\n",reg,reg,
(rb[0] << 8) | rb[1],(rb[0] << 8) | rb[1]);
--
2.39.2
Hi,
Some comments on this patch:
On 13/04/2023 11:21, Kernel-Development wrote:
> This is a patch that fixes a bug:
> KMSAN: uninit-value in dib3000mb_attach (2)
>
> Local variable u8 rb[2] is not initialized as it is used as read buffer
> for i2c_transfer(). It is expected that i2c_transfer() should fill in
> the buffer before the target function returns rb's content. However
> error handling of i2c_transfer is not done, and on occasions where the
> read fails, uninitialized rb value will be returned.
>
> The usage of this function, defined as macro rd() in
> drivers/media/dvb-frontends/dib3000mb_priv,h, does not expect any error
> to occur. Adding error handling here might involve significant code
> changes.
>
> Thus 0-initialization is done on rb. This might affect some logic on
> error case as the use of the return value is used as boolean and flags.
>
> Reported-by: [email protected]
> Link: https://syzkaller.appspot.com/bug?id=2f4d19de8c9e9f0b9794e53ca54d68e0ffe9f068
> Signed-off-by: (Ben) HokChun Ng <[email protected]>
> ---
> drivers/media/dvb-frontends/dib3000mb.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/dib3000mb.c b/drivers/media/dvb-frontends/dib3000mb.c
> index a6c2fc4586eb..0dd96656aaf4 100644
> --- a/drivers/media/dvb-frontends/dib3000mb.c
> +++ b/drivers/media/dvb-frontends/dib3000mb.c
> @@ -50,15 +50,19 @@ MODULE_PARM_DESC(debug, "set debugging level (1=info,2=xfer,4=setfe,8=getfe (|-a
>
> static int dib3000_read_reg(struct dib3000_state *state, u16 reg)
> {
> + int errno;
> u8 wb[] = { ((reg >> 8) | 0x80) & 0xff, reg & 0xff };
> - u8 rb[2];
> + u8 rb[2] = { 0, 0 };
Really all you need to do here is zero this array, which can be even
shorter by writing: u8 rb[2] = {};
It is enough to just show the "i2c read error" message, nothing else
is needed here.
BTW, checkpatch.pl also complains about your email address ('Kernel-Development <[email protected]>'
being different from your SoB line: (Ben) HokChun Ng <[email protected]>.
It's a good idea to ensure the two are the same. I would stick to
(Ben) HokChun Ng <[email protected]> since that has your actual name.
Regards,
Hans
> struct i2c_msg msg[] = {
> { .addr = state->config.demod_address, .flags = 0, .buf = wb, .len = 2 },
> { .addr = state->config.demod_address, .flags = I2C_M_RD, .buf = rb, .len = 2 },
> };
>
> - if (i2c_transfer(state->i2c, msg, 2) != 2)
> - deb_i2c("i2c read error\n");
> + errno = i2c_transfer(state->i2c, msg, 2);
> + if (errno != 2) {
> + deb_i2c("i2c read error (errno: %d)\n", -errno);
> + return 0;
> + }
>
> deb_i2c("reading i2c bus (reg: %5d 0x%04x, val: %5d 0x%04x)\n",reg,reg,
> (rb[0] << 8) | rb[1],(rb[0] << 8) | rb[1]);