| Message ID | 0100018779eb40dc-cee9e39d-5d87-4733-83db-eca5218fcc8f-000000@email.amazonses.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Initialization of read buffer for dib3000_read_reg | expand |
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: syzbot+c88fc0ebe0d5935c70da@syzkaller.appspotmail.com > Link: https://syzkaller.appspot.com/bug?id=2f4d19de8c9e9f0b9794e53ca54d68e0ffe9f068 > Signed-off-by: (Ben) HokChun Ng <kdev@benbenng.net> > --- > 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 <kdev@benbenng.net>' being different from your SoB line: (Ben) HokChun Ng <kdev@benbenng.net>. It's a good idea to ensure the two are the same. I would stick to (Ben) HokChun Ng <kdev@benbenng.net> 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]);
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]);
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: syzbot+c88fc0ebe0d5935c70da@syzkaller.appspotmail.com Link: https://syzkaller.appspot.com/bug?id=2f4d19de8c9e9f0b9794e53ca54d68e0ffe9f068 Signed-off-by: (Ben) HokChun Ng <kdev@benbenng.net> --- drivers/media/dvb-frontends/dib3000mb.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)