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,
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.
[ Sasha's backport helper bot ]
Hi,
✅ All tests passed successfully. No issues detected. No action required from the submitter.
The upstream commit SHA1 provided is correct: fa332f5dc6fc662ad7d3200048772c96b861cf6b
WARNING: Author mismatch between patch and upstream commit: Backport author: Brendan Jackmanjackmanb@google.com Commit author: Dan Carpenterdan.carpenter@linaro.org
Note: The patch differs from the upstream commit: --- 1: fa332f5dc6fc6 ! 1: 5984afb270d63 ipmi:msghandler: Fix potential memory corruption in ipmi_create_user() @@ Metadata ## Commit message ## ipmi:msghandler: Fix potential memory corruption in ipmi_create_user()
+ 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. @@ Commit message 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
## drivers/char/ipmi/ipmi_msghandler.c ## @@ drivers/char/ipmi/ipmi_msghandler.c: int ipmi_create_user(unsigned int if_num, @@ drivers/char/ipmi/ipmi_msghandler.c: int ipmi_create_user(unsigned int + goto out_unlock;
found: - if (intf->in_shutdown) { - rv = -ENODEV; -- goto out_kfree; -+ goto out_unlock; - } - if (atomic_add_return(1, &intf->nr_users) > max_users) { @@ drivers/char/ipmi/ipmi_msghandler.c: int ipmi_create_user(unsigned int if_num, - } else { - *user = new_user; - } + + out_kfree: + atomic_dec(&intf->nr_users); +out_unlock: - mutex_unlock(&ipmi_interfaces_mutex); + srcu_read_unlock(&ipmi_interfaces_srcu, index); + vfree(new_user); return rv; - } ---
Results of testing on various branches:
| Branch | Patch Apply | Build Test | |---------------------------|-------------|------------| | stable/linux-6.15.y | Success | Success | | stable/linux-6.12.y | Success | Success | | stable/linux-6.6.y | Success | Success | | stable/linux-6.1.y | Success | Success | | stable/linux-5.15.y | Success | Success | | stable/linux-5.10.y | Success | Success | | stable/linux-5.4.y | Success | Success |
linux-stable-mirror@lists.linaro.org
-
Brendan Jackman -
Greg KH -
Sasha Levin