Initialization of read buffer for dib3000_read_reg
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/1 Initialization of read buffer for dib3000_read_reg
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.
--- drivers/media/dvb-frontends/dib3000mb.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
drivers/media/dvb-frontends/dib3000mb.c +7 -3
@@ -50,15 +50,19 @@ MODULE_PARM_DESC(debug, "set debugging level (1=info,2=xfer,4=setfe,8=getfe (|-astatic 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 ]);
Signature · 1 line
2.39.2
Hi,
Some comments on this patch:
Quoted 29 lines · Kernel-Development
This is a patch that fixes a bug:
KMSAN: uninit-value indib3000mb_attach(2)Local variable u8 rb[2] is not initialized as it is used as read buffer for
i2c_transfer(). It is expected thati2c_transfer()should fill in the buffer before the target function returns rb's content. However error handling ofi2c_transferis not done, and on occasions where the read fails, uninitialized rb value will be returned.The usage of this function, defined as macro
rd()indrivers/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]Signed-off-by (Ben) HokChun Ng <[email protected]>--- drivers/media/dvb-frontends/dib3000mb.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)drivers/media/dvb-frontends/dib3000mb.c +0
-0
drivers/media/dvb-frontends/dib3000mb.c MODULE_PARM_DESC(debug, "set debugging level (1=info,2=xfer,4=setfe,8=getfe (|-a · line 55
Quoted 7 lines
drivers/media/dvb-frontends/dib3000mb.c +2
-1@@ -50,15 +50,19 @@ MODULE_PARM_DESC(debug, "set debugging level (1=info,2=xfer,4=setfe,8=getfe (|-astatic 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
Quoted 15 lines
drivers/media/dvb-frontends/dib3000mb.c +5
-2struct 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 ]);