From: Dan Carpenter <dan.carpenter@linaro.org>

commit fa332f5dc6fc662ad7d3200048772c96b861cf6b upstream

The "intf" list iterator is an invalid pointer if the correct
"intf->intf_num" is not found.  Calling atomic_dec(&intf->nr_users) on
and invalid pointer will lead to memory corruption.

We don't really need to call atomic_dec() if we haven't called
atomic_add_return() so update the if (intf->in_shutdown) path as well.

Fixes: 8e76741c3d8b ("ipmi: Add a limit on the number of users that may use IPMI")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
Message-ID: <aBjMZ8RYrOt6NOgi@stanley.mountain>
Signed-off-by: Corey Minyard <corey@minyard.net>
Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
I have tested this in 6.12 with Google's platform drivers added to
reproduce the bug.  The bug causes the panic notifier chain to get
corrupted leading to a crash. With the fix this goes away.

Applies to 6.6 too but I haven't tested it there.

Backport changes:

- Dropped change to the `if (intf->in_shutdown)` block since that logic
  doesn't exist yet.
- Modified out_unlock to release the srcu lock instead of the mutex
  since we don't have the mutex here yet.
---
 drivers/char/ipmi/ipmi_msghandler.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index e12b531f5c2f338008a42dc2c35b0a62395c9f3c..6a4a8ecd0edd02793eda70f9f9ae578e37da477f 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -1241,7 +1241,7 @@ int ipmi_create_user(unsigned int          if_num,
 	}
 	/* Not found, return an error */
 	rv = -EINVAL;
-	goto out_kfree;
+	goto out_unlock;
 
  found:
 	if (atomic_add_return(1, &intf->nr_users) > max_users) {
@@ -1283,6 +1283,7 @@ int ipmi_create_user(unsigned int          if_num,
 
 out_kfree:
 	atomic_dec(&intf->nr_users);
+out_unlock:
 	srcu_read_unlock(&ipmi_interfaces_srcu, index);
 	vfree(new_user);
 	return rv;

---
base-commit: 783cd2c3dca8b6c434e955b84c20c8940588dc68
change-id: 20250630-ipmi-fix-c565f7098afd

Best regards,
-- 
Brendan Jackman <jackmanb@google.com>

On Mon, Jun 30, 2025 at 05:09:02PM +0000, Brendan Jackman wrote:
> From: Dan Carpenter <dan.carpenter@linaro.org>
> 
> commit fa332f5dc6fc662ad7d3200048772c96b861cf6b upstream
> 
> The "intf" list iterator is an invalid pointer if the correct
> "intf->intf_num" is not found.  Calling atomic_dec(&intf->nr_users) on
> and invalid pointer will lead to memory corruption.
> 
> We don't really need to call atomic_dec() if we haven't called
> atomic_add_return() so update the if (intf->in_shutdown) path as well.
> 
> Fixes: 8e76741c3d8b ("ipmi: Add a limit on the number of users that may use IPMI")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> Message-ID: <aBjMZ8RYrOt6NOgi@stanley.mountain>
> Signed-off-by: Corey Minyard <corey@minyard.net>
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---
> I have tested this in 6.12 with Google's platform drivers added to
> reproduce the bug.  The bug causes the panic notifier chain to get
> corrupted leading to a crash. With the fix this goes away.
> 
> Applies to 6.6 too but I haven't tested it there.

So what kernels are you wanting this to be applied to?

thanks,

greg k-h

On Mon Jun 30, 2025 at 6:10 PM UTC, Greg KH wrote:
> On Mon, Jun 30, 2025 at 05:09:02PM +0000, Brendan Jackman wrote:
>> From: Dan Carpenter <dan.carpenter@linaro.org>
>> 
>> commit fa332f5dc6fc662ad7d3200048772c96b861cf6b upstream
>> 
>> The "intf" list iterator is an invalid pointer if the correct
>> "intf->intf_num" is not found.  Calling atomic_dec(&intf->nr_users) on
>> and invalid pointer will lead to memory corruption.
>> 
>> We don't really need to call atomic_dec() if we haven't called
>> atomic_add_return() so update the if (intf->in_shutdown) path as well.
>> 
>> Fixes: 8e76741c3d8b ("ipmi: Add a limit on the number of users that may use IPMI")
>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Message-ID: <aBjMZ8RYrOt6NOgi@stanley.mountain>
>> Signed-off-by: Corey Minyard <corey@minyard.net>
>> Signed-off-by: Brendan Jackman <jackmanb@google.com>
>> ---
>> I have tested this in 6.12 with Google's platform drivers added to
>> reproduce the bug.  The bug causes the panic notifier chain to get
>> corrupted leading to a crash. With the fix this goes away.
>> 
>> Applies to 6.6 too but I haven't tested it there.
>
> So what kernels are you wanting this to be applied to?

Right, sorry for the ambiguity.  I've just applied the patch to 6.6 and
booted QEMU and it worked fine.

I have not reproduced a crash in 6.6 but it's pretty clearly a real bug
(it decrements the target of an uninitialized pointer).

So if you're OK with that then please apply to 6.6 and 6.12. Otherwise
just 6.12 is fine, I will send another PATCH if I ever hit the issue for
real in 6.6.

On Tue, Jul 01, 2025 at 09:52:55AM +0000, Brendan Jackman wrote:
> On Mon Jun 30, 2025 at 6:10 PM UTC, Greg KH wrote:
> > On Mon, Jun 30, 2025 at 05:09:02PM +0000, Brendan Jackman wrote:
> >> From: Dan Carpenter <dan.carpenter@linaro.org>
> >> 
> >> commit fa332f5dc6fc662ad7d3200048772c96b861cf6b upstream
> >> 
> >> The "intf" list iterator is an invalid pointer if the correct
> >> "intf->intf_num" is not found.  Calling atomic_dec(&intf->nr_users) on
> >> and invalid pointer will lead to memory corruption.
> >> 
> >> We don't really need to call atomic_dec() if we haven't called
> >> atomic_add_return() so update the if (intf->in_shutdown) path as well.
> >> 
> >> Fixes: 8e76741c3d8b ("ipmi: Add a limit on the number of users that may use IPMI")
> >> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> >> Message-ID: <aBjMZ8RYrOt6NOgi@stanley.mountain>
> >> Signed-off-by: Corey Minyard <corey@minyard.net>
> >> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> >> ---
> >> I have tested this in 6.12 with Google's platform drivers added to
> >> reproduce the bug.  The bug causes the panic notifier chain to get
> >> corrupted leading to a crash. With the fix this goes away.
> >> 
> >> Applies to 6.6 too but I haven't tested it there.
> >
> > So what kernels are you wanting this to be applied to?
> 
> Right, sorry for the ambiguity.  I've just applied the patch to 6.6 and
> booted QEMU and it worked fine.
> 
> I have not reproduced a crash in 6.6 but it's pretty clearly a real bug
> (it decrements the target of an uninitialized pointer).
> 
> So if you're OK with that then please apply to 6.6 and 6.12. Otherwise
> just 6.12 is fine, I will send another PATCH if I ever hit the issue for
> real in 6.6.

But why would we skip 6.15.y?  You can't apply patches to only older
stable kernels, as that would cause users to have regressions when they
move to newer ones. :(

greg k-h

>

On Tue Jul 1, 2025 at 10:14 AM UTC, Greg KH wrote:
> On Tue, Jul 01, 2025 at 09:52:55AM +0000, Brendan Jackman wrote:
>> On Mon Jun 30, 2025 at 6:10 PM UTC, Greg KH wrote:
>> > On Mon, Jun 30, 2025 at 05:09:02PM +0000, Brendan Jackman wrote:
>> >> From: Dan Carpenter <dan.carpenter@linaro.org>
>> >> 
>> >> commit fa332f5dc6fc662ad7d3200048772c96b861cf6b upstream
>> >> 
>> >> The "intf" list iterator is an invalid pointer if the correct
>> >> "intf->intf_num" is not found.  Calling atomic_dec(&intf->nr_users) on
>> >> and invalid pointer will lead to memory corruption.
>> >> 
>> >> We don't really need to call atomic_dec() if we haven't called
>> >> atomic_add_return() so update the if (intf->in_shutdown) path as well.
>> >> 
>> >> Fixes: 8e76741c3d8b ("ipmi: Add a limit on the number of users that may use IPMI")
>> >> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>> >> Message-ID: <aBjMZ8RYrOt6NOgi@stanley.mountain>
>> >> Signed-off-by: Corey Minyard <corey@minyard.net>
>> >> Signed-off-by: Brendan Jackman <jackmanb@google.com>
>> >> ---
>> >> I have tested this in 6.12 with Google's platform drivers added to
>> >> reproduce the bug.  The bug causes the panic notifier chain to get
>> >> corrupted leading to a crash. With the fix this goes away.
>> >> 
>> >> Applies to 6.6 too but I haven't tested it there.
>> >
>> > So what kernels are you wanting this to be applied to?
>> 
>> Right, sorry for the ambiguity.  I've just applied the patch to 6.6 and
>> booted QEMU and it worked fine.
>> 
>> I have not reproduced a crash in 6.6 but it's pretty clearly a real bug
>> (it decrements the target of an uninitialized pointer).
>> 
>> So if you're OK with that then please apply to 6.6 and 6.12. Otherwise
>> just 6.12 is fine, I will send another PATCH if I ever hit the issue for
>> real in 6.6.
>
> But why would we skip 6.15.y?  

Ah, sorry. I forgot that existed (I was conflating "longterm" and
"stable" in my mind).

I've just applied this patch to 6.15.y and booted QEMU, seems OK.

Aaand now this made me realise I also forgot 6.1 existed and it also
has the bug. So I've just applied it to 6.1.y and booted that in QEMU
too.

So: 6.15, 6.12, 6.6, and 6.1.