Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1673442 > unrolled thread
| Started by | Seunghun Han <kkamagui@gmail.com> |
|---|---|
| First post | 2017-06-23 11:40 +0200 |
| Last post | 2017-06-26 07:30 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH V4] acpi: acpica: fix acpi parse and parseext cache leaks Seunghun Han <kkamagui@gmail.com> - 2017-06-23 11:40 +0200
Re: [PATCH V4] acpi: acpica: fix acpi parse and parseext cache leaks Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-25 18:20 +0200
Re: [PATCH V4] acpi: acpica: fix acpi parse and parseext cache leaks Seunghun Han <kkamagui@gmail.com> - 2017-06-26 07:30 +0200
| From | Seunghun Han <kkamagui@gmail.com> |
|---|---|
| Date | 2017-06-23 11:40 +0200 |
| Subject | [PATCH V4] acpi: acpica: fix acpi parse and parseext cache leaks |
| Message-ID | <tVtei-oa-27@gated-at.bofh.it> |
I'm Seunghun Han, and I work for National Security Research Institute of
South Korea.
I have been doing a research on ACPI and found an ACPI cache leak in ACPI
early abort cases.
Boot log of ACPI cache leak is as follows:
[ 0.352414] ACPI: Added _OSI(Module Device)
[ 0.353182] ACPI: Added _OSI(Processor Device)
[ 0.353182] ACPI: Added _OSI(3.0 _SCP Extensions)
[ 0.353182] ACPI: Added _OSI(Processor Aggregator Device)
[ 0.356028] ACPI: Unable to start the ACPI Interpreter
[ 0.356799] ACPI Error: Could not remove SCI handler (20170303/evmisc-281)
[ 0.360215] kmem_cache_destroy Acpi-State: Slab cache still has objects
[ 0.360648] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W
4.12.0-rc4-next-20170608+ #10
[ 0.361273] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS
VirtualBox 12/01/2006
[ 0.361873] Call Trace:
[ 0.362243] ? dump_stack+0x5c/0x81
[ 0.362591] ? kmem_cache_destroy+0x1aa/0x1c0
[ 0.362944] ? acpi_sleep_proc_init+0x27/0x27
[ 0.363296] ? acpi_os_delete_cache+0xa/0x10
[ 0.363646] ? acpi_ut_delete_caches+0x6d/0x7b
[ 0.364000] ? acpi_terminate+0xa/0x14
[ 0.364000] ? acpi_init+0x2af/0x34f
[ 0.364000] ? __class_create+0x4c/0x80
[ 0.364000] ? video_setup+0x7f/0x7f
[ 0.364000] ? acpi_sleep_proc_init+0x27/0x27
[ 0.364000] ? do_one_initcall+0x4e/0x1a0
[ 0.364000] ? kernel_init_freeable+0x189/0x20a
[ 0.364000] ? rest_init+0xc0/0xc0
[ 0.364000] ? kernel_init+0xa/0x100
[ 0.364000] ? ret_from_fork+0x25/0x30
I analyzed this memory leak in detail. I found that “Acpi-State” cache and
“Acpi-Parse” cache were merged because the size of cache objects was same
slab cache size.
I finally found “Acpi-Parse” cache and “Acpi-ParseExt” cache were leaked
using SLAB_NEVER_MERGE flag in kmem_cache_create() function.
Real ACPI cache leak point is as follows:
[ 0.360101] ACPI: Added _OSI(Module Device)
[ 0.360101] ACPI: Added _OSI(Processor Device)
[ 0.360101] ACPI: Added _OSI(3.0 _SCP Extensions)
[ 0.361043] ACPI: Added _OSI(Processor Aggregator Device)
[ 0.364016] ACPI: Unable to start the ACPI Interpreter
[ 0.365061] ACPI Error: Could not remove SCI handler (20170303/evmisc-281)
[ 0.368174] kmem_cache_destroy Acpi-Parse: Slab cache still has objects
[ 0.369332] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G W
4.12.0-rc4-next-20170608+ #8
[ 0.371256] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS
VirtualBox 12/01/2006
[ 0.372000] Call Trace:
[ 0.372000] ? dump_stack+0x5c/0x81
[ 0.372000] ? kmem_cache_destroy+0x1aa/0x1c0
[ 0.372000] ? acpi_sleep_proc_init+0x27/0x27
[ 0.372000] ? acpi_os_delete_cache+0xa/0x10
[ 0.372000] ? acpi_ut_delete_caches+0x56/0x7b
[ 0.372000] ? acpi_terminate+0xa/0x14
[ 0.372000] ? acpi_init+0x2af/0x34f
[ 0.372000] ? __class_create+0x4c/0x80
[ 0.372000] ? video_setup+0x7f/0x7f
[ 0.372000] ? acpi_sleep_proc_init+0x27/0x27
[ 0.372000] ? do_one_initcall+0x4e/0x1a0
[ 0.372000] ? kernel_init_freeable+0x189/0x20a
[ 0.372000] ? rest_init+0xc0/0xc0
[ 0.372000] ? kernel_init+0xa/0x100
[ 0.372000] ? ret_from_fork+0x25/0x30
[ 0.388039] kmem_cache_destroy Acpi-ParseExt: Slab cache still has objects
[ 0.389063] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G W
4.12.0-rc4-next-20170608+ #8
[ 0.390557] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS
VirtualBox 12/01/2006
[ 0.392000] Call Trace:
[ 0.392000] ? dump_stack+0x5c/0x81
[ 0.392000] ? kmem_cache_destroy+0x1aa/0x1c0
[ 0.392000] ? acpi_sleep_proc_init+0x27/0x27
[ 0.392000] ? acpi_os_delete_cache+0xa/0x10
[ 0.392000] ? acpi_ut_delete_caches+0x6d/0x7b
[ 0.392000] ? acpi_terminate+0xa/0x14
[ 0.392000] ? acpi_init+0x2af/0x34f
[ 0.392000] ? __class_create+0x4c/0x80
[ 0.392000] ? video_setup+0x7f/0x7f
[ 0.392000] ? acpi_sleep_proc_init+0x27/0x27
[ 0.392000] ? do_one_initcall+0x4e/0x1a0
[ 0.392000] ? kernel_init_freeable+0x189/0x20a
[ 0.392000] ? rest_init+0xc0/0xc0
[ 0.392000] ? kernel_init+0xa/0x100
[ 0.392000] ? ret_from_fork+0x25/0x30
When early abort is occurred due to invalid ACPI information, Linux kernel
terminates ACPI by calling acpi_terminate() function. The function calls
acpi_ut_delete_caches() function to delete local caches (acpi_gbl_namespace_
cache, state_cache, operand_cache, ps_node_cache, ps_node_ext_cache).
But the deletion codes in acpi_ut_delete_caches() function only delete
slab caches using kmem_cache_destroy() function, therefore the cache
objects should be flushed before acpi_ut_delete_caches() function.
“Acpi-Parse” cache and “Acpi-ParseExt” cache are used in an AML parse
function, acpi_ps_parse_loop(). The function should have flush codes to
handle an error state due to invalid AML codes.
This cache leak has a security threat because an old kernel (<= 4.9) shows
memory locations of kernel functions in stack dump. Some malicious users
could use this information to neutralize kernel ASLR.
To fix ACPI cache leak for enhancing security, I made a patch which has
flush codes in acpi_ps_parse_loop() function.
I hope that this patch improves the security of Linux kernel.
Thank you.
Signed-off-by: Seunghun Han <kkamagui@gmail.com>
---
Changes since v3: change control transfer according to reviewer's comments.
Changes since v2: merge flush code with existing code and change comments.
Changes since v1: move flush code to acpi_ps_complete_final_op() function.
drivers/acpi/acpica/psobject.c | 53 +++++++++++++-----------------------------
1 file changed, 16 insertions(+), 37 deletions(-)
diff --git a/drivers/acpi/acpica/psobject.c b/drivers/acpi/acpica/psobject.c
index 5bcb618..4539391 100644
--- a/drivers/acpi/acpica/psobject.c
+++ b/drivers/acpi/acpica/psobject.c
@@ -608,7 +608,8 @@ acpi_status
acpi_ps_complete_final_op(struct acpi_walk_state *walk_state,
union acpi_parse_object *op, acpi_status status)
{
- acpi_status status2;
+ acpi_status return_status = AE_OK;
+ u8 ascending = TRUE;
ACPI_FUNCTION_TRACE_PTR(ps_complete_final_op, walk_state);
@@ -622,7 +623,8 @@ acpi_ps_complete_final_op(struct acpi_walk_state *walk_state,
op));
do {
if (op) {
- if (walk_state->ascending_callback != NULL) {
+ if (ascending &&
+ walk_state->ascending_callback != NULL) {
walk_state->op = op;
walk_state->op_info =
acpi_ps_get_opcode_info(op->common.
@@ -644,49 +646,26 @@ acpi_ps_complete_final_op(struct acpi_walk_state *walk_state,
}
if (status == AE_CTRL_TERMINATE) {
- status = AE_OK;
-
- /* Clean up */
- do {
- if (op) {
- status2 =
- acpi_ps_complete_this_op
- (walk_state, op);
- if (ACPI_FAILURE
- (status2)) {
- return_ACPI_STATUS
- (status2);
- }
- }
-
- acpi_ps_pop_scope(&
- (walk_state->
- parser_state),
- &op,
- &walk_state->
- arg_types,
- &walk_state->
- arg_count);
-
- } while (op);
-
- return_ACPI_STATUS(status);
+ ascending = FALSE;
+ return_status = AE_CTRL_TERMINATE;
}
else if (ACPI_FAILURE(status)) {
/* First error is most important */
- (void)
- acpi_ps_complete_this_op(walk_state,
- op);
- return_ACPI_STATUS(status);
+ ascending = FALSE;
+ return_status = status;
}
}
- status2 = acpi_ps_complete_this_op(walk_state, op);
- if (ACPI_FAILURE(status2)) {
- return_ACPI_STATUS(status2);
+ status = acpi_ps_complete_this_op(walk_state, op);
+ if (ACPI_FAILURE(status)) {
+ ascending = FALSE;
+ if (ACPI_SUCCESS(return_status) ||
+ return_status == AE_CTRL_TERMINATE) {
+ return_status = status;
+ }
}
}
@@ -696,5 +675,5 @@ acpi_ps_complete_final_op(struct acpi_walk_state *walk_state,
} while (op);
- return_ACPI_STATUS(status);
+ return_ACPI_STATUS(return_status);
}
--
2.1.4
[toc] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2017-06-25 18:20 +0200 |
| Message-ID | <tWiqt-7jf-3@gated-at.bofh.it> |
| In reply to | #1673442 |
On Fri, Jun 23, 2017 at 12:36 PM, Seunghun Han <kkamagui@gmail.com> wrote:
> I'm Seunghun Han, and I work for National Security Research Institute of
> South Korea.
> - /* Clean up */
> - do {
> - if (op) {
> - status2 =
> - acpi_ps_complete_this_op
> - (walk_state, op);
> - if (ACPI_FAILURE
> - (status2)) {
> - return_ACPI_STATUS
> - (status2);
> - }
> - }
> -
> - acpi_ps_pop_scope(&
> - (walk_state->
> - parser_state),
> - &op,
> - &walk_state->
> - arg_types,
> - &walk_state->
> - arg_count);
> -
> - } while (op);
I didn't get, do you send an incremental patch as a new version?!
--
With Best Regards,
Andy Shevchenko
[toc] | [prev] | [next] | [standalone]
| From | Seunghun Han <kkamagui@gmail.com> |
|---|---|
| Date | 2017-06-26 07:30 +0200 |
| Message-ID | <tWuL0-6CP-5@gated-at.bofh.it> |
| In reply to | #1674248 |
Hello, Andy.
Thank you for your reply.
Patch V4 is the last patch which is applied all changes from original code.
Due to maintainer's advice, Patch V1, V2, V3 have reverted and I made
the latest patch, Patch V4.
(the review is here, https://github.com/acpica/acpica/pull/278)
Therefore, it seems that incremental patch is not needed.
If you have any request, please let me know.
Best regards.
ps) I am sending email again after removing HTML part.
2017-06-26 7:12 GMT+09:00 Seunghun Han <kkamagui@gmail.com>:
> Hello, Andy.
>
> Thank you for your reply.
>
> Patch V4 is the last patch which is applied all changes from original code.
> Due to reviewer's advice, Patch V1, V2, V3 has reverted and I made the
> latest patch, Patch V4.
> Therefore, it seems that incremental patch is not needed.
>
> If you have any requests, please let me know.
>
> Best regards.
>
>
> 2017년 6월 26일 (월) 오전 1:13, Andy Shevchenko <andy.shevchenko@gmail.com>님이 작성:
>>
>> On Fri, Jun 23, 2017 at 12:36 PM, Seunghun Han <kkamagui@gmail.com> wrote:
>> > I'm Seunghun Han, and I work for National Security Research Institute of
>> > South Korea.
>>
>>
>> > - /* Clean up */
>> > - do {
>> > - if (op) {
>> > - status2 =
>> > -
>> > acpi_ps_complete_this_op
>> > - (walk_state,
>> > op);
>> > - if (ACPI_FAILURE
>> > - (status2)) {
>> > -
>> > return_ACPI_STATUS
>> > -
>> > (status2);
>> > - }
>> > - }
>> > -
>> > - acpi_ps_pop_scope(&
>> > -
>> > (walk_state->
>> > -
>> > parser_state),
>> > - &op,
>> > -
>> > &walk_state->
>> > -
>> > arg_types,
>> > -
>> > &walk_state->
>> > -
>> > arg_count);
>> > -
>> > - } while (op);
>>
>> I didn't get, do you send an incremental patch as a new version?!
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web