2024-06-14 17:36:01

by Dan Carpenter

Subject: [PATCH] ipmi: ssif_bmc: prevent integer overflow on 32bit systems

There are actually two bugs here. First, we need to ensure that count
is at least sizeof(u32) or msg.len will be uninitialized data.

The "msg.len" variable is a u32 that comes from the user. On 32bit
systems the "sizeof_field(struct ipmi_ssif_msg, len) + msg.len"
addition can overflow if "msg.len" is greater than U32_MAX - 4.

Valid lengths for "msg.len" are 1-254. Add a check for that to
prevent the integer overflow.

Fixes: dd2bc5cc9e25 ("ipmi: ssif_bmc: Add SSIF BMC driver")
Signed-off-by: Dan Carpenter <[email protected]>
---
drivers/char/ipmi/ssif_bmc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
---
drivers/char/ipmi/ssif_bmc.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
index 56346fb32872..ab4e87a99f08 100644
--- a/drivers/char/ipmi/ssif_bmc.c
+++ b/drivers/char/ipmi/ssif_bmc.c
@@ -177,13 +177,15 @@ static ssize_t ssif_bmc_write(struct file *file, const char __user *buf, size_t
unsigned long flags;
ssize_t ret;

- if (count > sizeof(struct ipmi_ssif_msg))
+ if (count < sizeof(msg.len) ||
+ count > sizeof(struct ipmi_ssif_msg))
return -EINVAL;

if (copy_from_user(&msg, buf, count))
return -EFAULT;

- if (!msg.len || count < sizeof_field(struct ipmi_ssif_msg, len) + msg.len)
+ if (!msg.len || msg.len > IPMI_SSIF_PAYLOAD_MAX ||
+ count < sizeof_field(struct ipmi_ssif_msg, len) + msg.len)
return -EINVAL;

spin_lock_irqsave(&ssif_bmc->lock, flags);
--
2.43.0

2024-06-14 21:56:58

by Corey Minyard

Subject: Re: [PATCH] ipmi: ssif_bmc: prevent integer overflow on 32bit systems

On Fri, Jun 14, 2024 at 08:30:44PM +0300, Dan Carpenter wrote:
> There are actually two bugs here. First, we need to ensure that count
> is at least sizeof(u32) or msg.len will be uninitialized data.
>
> The "msg.len" variable is a u32 that comes from the user. On 32bit
> systems the "sizeof_field(struct ipmi_ssif_msg, len) + msg.len"
> addition can overflow if "msg.len" is greater than U32_MAX - 4.
>
> Valid lengths for "msg.len" are 1-254. Add a check for that to
> prevent the integer overflow.

Thanks, this is in my tree.

-corey

>
> Fixes: dd2bc5cc9e25 ("ipmi: ssif_bmc: Add SSIF BMC driver")
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> drivers/char/ipmi/ssif_bmc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> ---
> drivers/char/ipmi/ssif_bmc.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
> index 56346fb32872..ab4e87a99f08 100644
> --- a/drivers/char/ipmi/ssif_bmc.c
> +++ b/drivers/char/ipmi/ssif_bmc.c
> @@ -177,13 +177,15 @@ static ssize_t ssif_bmc_write(struct file *file, const char __user *buf, size_t
> unsigned long flags;
> ssize_t ret;
>
> - if (count > sizeof(struct ipmi_ssif_msg))
> + if (count < sizeof(msg.len) ||
> + count > sizeof(struct ipmi_ssif_msg))
> return -EINVAL;
>
> if (copy_from_user(&msg, buf, count))
> return -EFAULT;
>
> - if (!msg.len || count < sizeof_field(struct ipmi_ssif_msg, len) + msg.len)
> + if (!msg.len || msg.len > IPMI_SSIF_PAYLOAD_MAX ||
> + count < sizeof_field(struct ipmi_ssif_msg, len) + msg.len)
> return -EINVAL;
>
> spin_lock_irqsave(&ssif_bmc->lock, flags);
> --
> 2.43.0
>