ipmi: ipmb: initialise event handler read bytes

[PATCH] ipmi: ipmb: initialise event handler read bytes

Posted by Matt Johnston 3 weeks, 4 days ago

IPMB doesn't use i2c reads, but the handler needs to set a value.
Otherwise an i2c read will return an uninitialised value from the bus
driver.

Fixes: 63c4eb347164 ("ipmi:ipmb: Add initial support for IPMI over IPMB")
Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
---
 drivers/char/ipmi/ipmi_ipmb.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/char/ipmi/ipmi_ipmb.c b/drivers/char/ipmi/ipmi_ipmb.c
index 3a51e58b24875497699c7af7a4af1c73b47653f3..28818952a7a4bf814b95bdfb7046672ad4526cf2 100644
--- a/drivers/char/ipmi/ipmi_ipmb.c
+++ b/drivers/char/ipmi/ipmi_ipmb.c
@@ -202,11 +202,16 @@ static int ipmi_ipmb_slave_cb(struct i2c_client *client,
 		break;
 
 	case I2C_SLAVE_READ_REQUESTED:
+		*val = 0xff;
+		ipmi_ipmb_check_msg_done(iidev);
+		break;
+
 	case I2C_SLAVE_STOP:
 		ipmi_ipmb_check_msg_done(iidev);
 		break;
 
 	case I2C_SLAVE_READ_PROCESSED:
+		*val = 0xff;
 		break;
 	}
 

---
base-commit: b71e635feefc852405b14620a7fc58c4c80c0f73
change-id: 20260113-ipmb-read-init-5b97dfbcd3b9

Best regards,
-- 
Matt Johnston <matt@codeconstruct.com.au>

Re: [PATCH] ipmi: ipmb: initialise event handler read bytes

Posted by Corey Minyard 3 weeks, 4 days ago

On Tue, Jan 13, 2026 at 05:41:34PM +0800, Matt Johnston wrote:
> IPMB doesn't use i2c reads, but the handler needs to set a value.
> Otherwise an i2c read will return an uninitialised value from the bus
> driver.

This is fine, I suppose.  It's probably better to do this.

Are you actually using this code?  How was it found?

-corey

> 
> Fixes: 63c4eb347164 ("ipmi:ipmb: Add initial support for IPMI over IPMB")
> Signed-off-by: Matt Johnston <matt@codeconstruct.com.au>
> ---
>  drivers/char/ipmi/ipmi_ipmb.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/char/ipmi/ipmi_ipmb.c b/drivers/char/ipmi/ipmi_ipmb.c
> index 3a51e58b24875497699c7af7a4af1c73b47653f3..28818952a7a4bf814b95bdfb7046672ad4526cf2 100644
> --- a/drivers/char/ipmi/ipmi_ipmb.c
> +++ b/drivers/char/ipmi/ipmi_ipmb.c
> @@ -202,11 +202,16 @@ static int ipmi_ipmb_slave_cb(struct i2c_client *client,
>  		break;
>  
>  	case I2C_SLAVE_READ_REQUESTED:
> +		*val = 0xff;
> +		ipmi_ipmb_check_msg_done(iidev);
> +		break;
> +
>  	case I2C_SLAVE_STOP:
>  		ipmi_ipmb_check_msg_done(iidev);
>  		break;
>  
>  	case I2C_SLAVE_READ_PROCESSED:
> +		*val = 0xff;
>  		break;
>  	}
>  
> 
> ---
> base-commit: b71e635feefc852405b14620a7fc58c4c80c0f73
> change-id: 20260113-ipmb-read-init-5b97dfbcd3b9
> 
> Best regards,
> -- 
> Matt Johnston <matt@codeconstruct.com.au>
>

Re: [PATCH] ipmi: ipmb: initialise event handler read bytes

Posted by Matt Johnston 3 weeks, 3 days ago

Hi Corey,

On Tue, 2026-01-13 at 10:00 -0600, Corey Minyard wrote:
> On Tue, Jan 13, 2026 at 05:41:34PM +0800, Matt Johnston wrote:
> > IPMB doesn't use i2c reads, but the handler needs to set a value.
> > Otherwise an i2c read will return an uninitialised value from the bus
> > driver.
> 
> This is fine, I suppose.  It's probably better to do this.
> 
> Are you actually using this code?  How was it found?

I'm not using IPMB, so haven't tested the patch on hardware.
We received a patch for mctp-i2c read handling, that made me notice 
the uninitialised value problem there. I had a look over other 
i2c_slave_register() kernel users to see if the problem was common
(mctp-i2c used ipmi-ipmb as a bit of a reference during development too).

https://lore.kernel.org/netdev/20260108101829.1140448-1-
zhangjian.3032@bytedance.com/
https://lore.kernel.org/netdev/20260113-mctp-read-fix-v1-1-
70c4b59c741c@codeconstruct.com.au/

Cheers,
Matt

Re: [PATCH] ipmi: ipmb: initialise event handler read bytes

Posted by Corey Minyard 3 weeks, 3 days ago

On Wed, Jan 14, 2026 at 10:10:34AM +0800, Matt Johnston wrote:
> Hi Corey,
> 
> On Tue, 2026-01-13 at 10:00 -0600, Corey Minyard wrote:
> > On Tue, Jan 13, 2026 at 05:41:34PM +0800, Matt Johnston wrote:
> > > IPMB doesn't use i2c reads, but the handler needs to set a value.
> > > Otherwise an i2c read will return an uninitialised value from the bus
> > > driver.
> > 
> > This is fine, I suppose.  It's probably better to do this.
> > 
> > Are you actually using this code?  How was it found?
> 
> I'm not using IPMB, so haven't tested the patch on hardware.
> We received a patch for mctp-i2c read handling, that made me notice 
> the uninitialised value problem there. I had a look over other 
> i2c_slave_register() kernel users to see if the problem was common
> (mctp-i2c used ipmi-ipmb as a bit of a reference during development too).

Ok, thanks for the history on this.  It's in my tree.

-corey

> 
> https://lore.kernel.org/netdev/20260108101829.1140448-1-
> zhangjian.3032@bytedance.com/
> https://lore.kernel.org/netdev/20260113-mctp-read-fix-v1-1-
> 70c4b59c741c@codeconstruct.com.au/
> 
> Cheers,
> Matt
>

Re: [PATCH] ipmi: ipmb: initialise event handler read bytes

Posted by Markus Elfring 3 weeks, 4 days ago

> IPMB doesn't use i2c reads, but the handler needs to set a value.
> Otherwise an i2c read will return an uninitialised value from the bus
> driver.

See also once more:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.19-rc5#n94

Regards,
Markus