[PATCH] acpi / apei: fix NULL deref during init

[PATCH] acpi / apei: fix NULL deref during init

[Thomas Schoebel-Theuer]

Since commit commit d91525eb8ee6 ("ACPI, EINJ: Enhance error injection
tolerance level"), starting with kernel 4.0, the following happens during
boot of a specific old hardware:

APEI: Can not request [mem 0x0009c2f2-0x0009c2fc] for APEI ERST registers
BUG: unable to handle kernel NULL pointer dereference at           (null)
IP: [<ffffffff81343d8e>] __list_del_entry+0x5c/0x98
PGD 0
Oops: 0000 [#1] SMP
Modules linked in:
CPU: 0 PID: 1 UID: 0 Comm: swapper/0 Not tainted 4.4.0-ui18344.004-uiabi1-infong-amd64 #1
Hardware name: IBM IBM eServer BladeCenter HS12 -[8028Z5S]-/Server Blade, BIOS -[N1E150AUS-1.11]- 11/04/2010
task: ffff88021fe4e040 ti: ffff88021fe7c000 task.ti: ffff88021fe7c000
RIP: 0010:[<ffffffff81343d8e>]  [<ffffffff81343d8e>] __list_del_entry+0x5c/0x98
RSP: 0000:ffff88021fe7fd18  EFLAGS: 00010207
RAX: 0000000000000000 RBX: ffff88021fe7fde0 RCX: ffff88021fe7fde0
RDX: ffffffff819bd040 RSI: dead000000000200 RDI: ffff88021fe7fde0
RBP: ffff88021fe7fd18 R08: 0000000000000000 R09: 0000000000000000
R10: ffffffff816ce240 R11: 0000000000000001 R12: ffffffff819bd040
R13: ffff88021fe7fda0 R14: ffff88021d2cd840 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff88022fc00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 00000000019b6000 CR4: 0000000000040670
Stack:
 ffff88021fe7fd30 ffffffff81343dd7 ffff88021fe7fde0 ffff88021fe7fd58
 ffffffff813931c0 ffff88021fe7fda0 ffff88021fe7fe00 ffff88021d2cd840
 ffff88021fe7fd70 ffffffff813931e5 00000000ffffffea ffff88021fe7fdf0
Call Trace:
 [<ffffffff81343dd7>] list_del+0xd/0x25
 [<ffffffff813931c0>] apei_res_clean+0x1f/0x37
 [<ffffffff813931e5>] apei_resources_fini+0xd/0x19
 [<ffffffff81393948>] apei_resources_request+0x24f/0x268
 [<ffffffff8139301d>] ? apei_exec_for_each_entry+0x77/0x8e
 [<ffffffff81b0f729>] ? setup_erst_disable+0x12/0x12
 [<ffffffff81b0f816>] erst_init+0xed/0x2ca
 [<ffffffff810003b2>] ? do_one_initcall+0x8c/0x174
 [<ffffffff81b0f729>] ? setup_erst_disable+0x12/0x12
 [<ffffffff81b0f729>] ? setup_erst_disable+0x12/0x12
 [<ffffffff8100040f>] do_one_initcall+0xe9/0x174
 [<ffffffff8105df00>] ? parse_args+0x161/0x296
 [<ffffffff81ad6070>] kernel_init_freeable+0x169/0x1f6
 [<ffffffff81ad579b>] ? do_early_param+0x88/0x88
 [<ffffffff81663202>] ? rest_init+0x79/0x79
 [<ffffffff8166320b>] kernel_init+0x9/0xd5
 [<ffffffff816683f5>] ret_from_fork+0x55/0x80
 [<ffffffff81663202>] ? rest_init+0x79/0x79
Code: 02 00 00 00 00 ad de 48 39 f0 75 1f 49 89 c0 48 c7 c2 38 de 8e 81 be 38 00 00 00 48 c7 c7 13 dd 8e 81 31 c0 e8 94 36 d0 ff eb 3a <48> 8b 30 48 39 fe 74 11 49 89 f0 48 c7 c2 6c de 8e 81 be 3b 00
RIP  [<ffffffff81343d8e>] __list_del_entry+0x5c/0x98
 RSP <ffff88021fe7fd18>
CR2: 0000000000000000
---[ end trace 3610e544cef27e81 ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009

Reason is a conditional initialization of variable arch_res, which happens
only under a specific precondition. When the condition is false, the
variable remains uninitialized.

This may later trigger a splat, e.g. when some error path is taken.

Solution: do the initialisation unconditionally. Also as a safeguard.

Fixes: d91525eb8ee6a622ce476955fe1a2530ade87c83
Signed-off-by: Thomas Schoebel-Theuer <tst@schoebel-theuer.de>
---
 drivers/acpi/apei/apei-base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index da370e1d31f4..ef931b8a0b11 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -494,8 +494,8 @@ int apei_resources_request(struct apei_resources *resources,
 	if (rc)
 		goto nvs_res_fini;
 
+	apei_resources_init(&arch_res);
 	if (arch_apei_filter_addr) {
-		apei_resources_init(&arch_res);
 		rc = apei_get_arch_resources(&arch_res);
 		if (rc)
 			goto arch_res_fini;
-- 
2.12.3

Re: [PATCH] acpi / apei: fix NULL deref during init

[Laura Abbott]

(adding some more people, please remember to run get_maintainer.pl
to get the full list in the future)
On 12/14/18 10:15 AM, Thomas Schoebel-Theuer wrote:
> Since commit commit d91525eb8ee6 ("ACPI, EINJ: Enhance error injection
> tolerance level"), starting with kernel 4.0, the following happens during
> boot of a specific old hardware:
> 
> APEI: Can not request [mem 0x0009c2f2-0x0009c2fc] for APEI ERST registers
> BUG: unable to handle kernel NULL pointer dereference at           (null)
> IP: [<ffffffff81343d8e>] __list_del_entry+0x5c/0x98
> PGD 0
> Oops: 0000 [#1] SMP
> Modules linked in:
> CPU: 0 PID: 1 UID: 0 Comm: swapper/0 Not tainted 4.4.0-ui18344.004-uiabi1-infong-amd64 #1
> Hardware name: IBM IBM eServer BladeCenter HS12 -[8028Z5S]-/Server Blade, BIOS -[N1E150AUS-1.11]- 11/04/2010
> task: ffff88021fe4e040 ti: ffff88021fe7c000 task.ti: ffff88021fe7c000
> RIP: 0010:[<ffffffff81343d8e>]  [<ffffffff81343d8e>] __list_del_entry+0x5c/0x98
> RSP: 0000:ffff88021fe7fd18  EFLAGS: 00010207
> RAX: 0000000000000000 RBX: ffff88021fe7fde0 RCX: ffff88021fe7fde0
> RDX: ffffffff819bd040 RSI: dead000000000200 RDI: ffff88021fe7fde0
> RBP: ffff88021fe7fd18 R08: 0000000000000000 R09: 0000000000000000
> R10: ffffffff816ce240 R11: 0000000000000001 R12: ffffffff819bd040
> R13: ffff88021fe7fda0 R14: ffff88021d2cd840 R15: 0000000000000000
> FS:  0000000000000000(0000) GS:ffff88022fc00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 00000000019b6000 CR4: 0000000000040670
> Stack:
>   ffff88021fe7fd30 ffffffff81343dd7 ffff88021fe7fde0 ffff88021fe7fd58
>   ffffffff813931c0 ffff88021fe7fda0 ffff88021fe7fe00 ffff88021d2cd840
>   ffff88021fe7fd70 ffffffff813931e5 00000000ffffffea ffff88021fe7fdf0
> Call Trace:
>   [<ffffffff81343dd7>] list_del+0xd/0x25
>   [<ffffffff813931c0>] apei_res_clean+0x1f/0x37
>   [<ffffffff813931e5>] apei_resources_fini+0xd/0x19
>   [<ffffffff81393948>] apei_resources_request+0x24f/0x268
>   [<ffffffff8139301d>] ? apei_exec_for_each_entry+0x77/0x8e
>   [<ffffffff81b0f729>] ? setup_erst_disable+0x12/0x12
>   [<ffffffff81b0f816>] erst_init+0xed/0x2ca
>   [<ffffffff810003b2>] ? do_one_initcall+0x8c/0x174
>   [<ffffffff81b0f729>] ? setup_erst_disable+0x12/0x12
>   [<ffffffff81b0f729>] ? setup_erst_disable+0x12/0x12
>   [<ffffffff8100040f>] do_one_initcall+0xe9/0x174
>   [<ffffffff8105df00>] ? parse_args+0x161/0x296
>   [<ffffffff81ad6070>] kernel_init_freeable+0x169/0x1f6
>   [<ffffffff81ad579b>] ? do_early_param+0x88/0x88
>   [<ffffffff81663202>] ? rest_init+0x79/0x79
>   [<ffffffff8166320b>] kernel_init+0x9/0xd5
>   [<ffffffff816683f5>] ret_from_fork+0x55/0x80
>   [<ffffffff81663202>] ? rest_init+0x79/0x79
> Code: 02 00 00 00 00 ad de 48 39 f0 75 1f 49 89 c0 48 c7 c2 38 de 8e 81 be 38 00 00 00 48 c7 c7 13 dd 8e 81 31 c0 e8 94 36 d0 ff eb 3a <48> 8b 30 48 39 fe 74 11 49 89 f0 48 c7 c2 6c de 8e 81 be 3b 00
> RIP  [<ffffffff81343d8e>] __list_del_entry+0x5c/0x98
>   RSP <ffff88021fe7fd18>
> CR2: 0000000000000000
> ---[ end trace 3610e544cef27e81 ]---
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009
> 
> Reason is a conditional initialization of variable arch_res, which happens
> only under a specific precondition. When the condition is false, the
> variable remains uninitialized.
> 
> This may later trigger a splat, e.g. when some error path is taken.
> 
> Solution: do the initialisation unconditionally. Also as a safeguard.
> 
> Fixes: d91525eb8ee6a622ce476955fe1a2530ade87c83
> Signed-off-by: Thomas Schoebel-Theuer <tst@schoebel-theuer.de>
> ---
>   drivers/acpi/apei/apei-base.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
> index da370e1d31f4..ef931b8a0b11 100644
> --- a/drivers/acpi/apei/apei-base.c
> +++ b/drivers/acpi/apei/apei-base.c
> @@ -494,8 +494,8 @@ int apei_resources_request(struct apei_resources *resources,
>   	if (rc)
>   		goto nvs_res_fini;
>   
> +	apei_resources_init(&arch_res);
>   	if (arch_apei_filter_addr) {
> -		apei_resources_init(&arch_res);
>   		rc = apei_get_arch_resources(&arch_res);
>   		if (rc)
>   			goto arch_res_fini;
> 

Re: [PATCH] acpi / apei: fix NULL deref during init

[Borislav Petkov]

On Fri, Dec 14, 2018 at 07:15:14PM +0100, Thomas Schoebel-Theuer wrote:
> Reason is a conditional initialization of variable arch_res, which happens
> only under a specific precondition. When the condition is false, the
> variable remains uninitialized.

Are you talking about the precondition

	if (arch_apei_filter_addr)

?

Because apei_resources_fini() happens under the same condition check and
if arch_apei_filter_addr was false, it should not become true, all of a
sudden. Or?

Or does that function ptr get set in the meantime on your machine? I.e., this
hackery:

#define set_apei_filter() (arch_apei_filter_addr = pci_mmcfg_for_each_region)

being called in pci_mmcfg_early_init()...

?

Hmmm.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] acpi / apei: fix NULL deref during init

[Thomas Schoebel-Theuer]

On 12/14/18 21:24, Borislav Petkov wrote:
>
> Because apei_resources_fini() happens under the same condition check and
> if arch_apei_filter_addr was false, it should not become true, all of a
> sudden. Or?

Hi Borislav,

please take a look at the stacktrace. For some reason, and only at that 
specific hardware, the condition is false, there but later the indicated 
error exit is taken whose message you can see immediately before the 
stack trace.

So this should documents the one observed case where the NULL deref is 
actually happening.

Of course, it would be possible to develop another solution, but this 
one appears the simplest and safest to me (minimum changes to the logic).

I have tested the patch on that specifc hardware: I have verified that 
the patch does not trigger the NULL deref anymore.

Of course, on any other hardware we have tested, the bug did not trigger 
at all.

If you don't have that specific hardware, you probably cannot easily 
trigger / verify the problem.

If you need access to the specfic hardware, talk to me in a private 
conversation.

Cheers,

Thomas

Re: [PATCH] acpi / apei: fix NULL deref during init

[Borislav Petkov]

On Fri, Dec 14, 2018 at 10:27:25PM +0100, Thomas Schoebel-Theuer wrote:
> please take a look at the stacktrace. For some reason, and only at that
> specific hardware, the condition is false, there but later the indicated
> error exit is taken whose message you can see immediately before the stack
> trace.

Yes, but if you say "for some reason", then we still don't know what
the root cause is. So before we do any fixing, let's find out what the
problem is first.

Can you pls run this debugging hunk ontop of -rc6, on the box and send
me full dmesg? Privately is fine too.

Thx.

---
diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index 7389db538c30..5166639b7280 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -667,6 +667,9 @@ void __init pci_mmcfg_early_init(void)
 			acpi_sfi_table_parse(ACPI_SIG_MCFG, pci_parse_mcfg);
 		__pci_mmcfg_init(1);
 
+		pr_info("%s: setting apei filter\n", __func__);
+		dump_stack();
+
 		set_apei_filter();
 	}
 }
diff --git a/drivers/acpi/apei/apei-base.c b/drivers/acpi/apei/apei-base.c
index da370e1d31f4..e87b183ca73d 100644
--- a/drivers/acpi/apei/apei-base.c
+++ b/drivers/acpi/apei/apei-base.c
@@ -494,6 +494,9 @@ int apei_resources_request(struct apei_resources *resources,
 	if (rc)
 		goto nvs_res_fini;
 
+	pr_info("%s: 1, arch_apei_filter_addr: 0x%px\n",
+		__func__, arch_apei_filter_addr);
+
 	if (arch_apei_filter_addr) {
 		apei_resources_init(&arch_res);
 		rc = apei_get_arch_resources(&arch_res);
@@ -552,6 +555,9 @@ int apei_resources_request(struct apei_resources *resources,
 		release_mem_region(res->start, res->end - res->start);
 	}
 arch_res_fini:
+	pr_info("%s: arch_res_fini, arch_apei_filter_addr: 0x%px\n",
+		__func__, arch_apei_filter_addr);
+
 	if (arch_apei_filter_addr)
 		apei_resources_fini(&arch_res);
 nvs_res_fini:

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] acpi / apei: fix NULL deref during init

[Thomas Schöbel-Theuer]

On 12/14/18 22:27, Thomas Schoebel-Theuer wrote:
> On 12/14/18 21:24, Borislav Petkov wrote:
>>
>> Because apei_resources_fini() happens under the same condition check and
>> if arch_apei_filter_addr was false, it should not become true, all of a
>> sudden. Or?
>
> please take a look at the stacktrace. For some reason, and only at 
> that specific hardware, the condition is false, there but later the 
> indicated error exit is taken whose message you can see immediately 
> before the stack trace.
>

Ah, I overlooked that commit e56c92565dfe2 is already providing a 
different solution to the same problem in newer kernels _only_, as a 
_side_ effect (not clear to me from the description, but clear from 
reading the code).

But this patch is not present at the 4.4.166 kernel where I found the 
problem and fixed it internally in a different way.

The 4.4.166 code looks like this, without the if-statement you are 
mentioning, unconditionally trying to free the unitinialized variable 
under certain circumstances:

d91525eb8ee6a (Chen, Gong     2014-12-10 13:53:26 -0800 553) arch_res_fini:
d91525eb8ee6a (Chen, Gong     2014-12-10 13:53:26 -0800 554) 
apei_resources_fini(&arch_res);
d91525eb8ee6a (Chen, Gong     2014-12-10 13:53:26 -0800 555) nvs_res_fini:
4134b8c8811f2 (Huang Ying     2011-12-08 11:25:50 +0800 556) 
apei_resources_fini(&nvs_resources);
23f124ca3dda9 (Huang Ying     2010-09-29 19:53:54 +0800 557) return rc;

So another alternative would be backporting e56c92565dfe2 to the 4.4 LTS 
series. Also fine for me.

Re: [PATCH] acpi / apei: fix NULL deref during init

[Borislav Petkov]

On Fri, Dec 14, 2018 at 11:42:01PM +0100, Thomas Schöbel-Theuer wrote:
> Ah, I overlooked that commit e56c92565dfe2 is already providing a different
> solution to the same problem in newer kernels _only_, as a _side_ effect
> (not clear to me from the description, but clear from reading the code).

Damn, I missed the fact that this is not the upstream kernel:

CPU: 0 PID: 1 UID: 0 Comm: swapper/0 Not tainted 4.4.0-ui18344.004-uiabi1-infong-amd64 #1

> So another alternative would be backporting e56c92565dfe2 to the 4.4 LTS
> series. Also fine for me.

That looks like the right fix.

A note for the next time: do not send a fix for a stable kernel which is
not upstream:

From Documentation/process/stable-kernel-rules.rst:

" - It or an equivalent fix must already exist in Linus' tree (upstream)."

The stable kernels track upstream so if a stable kernel has a problem,
the first thing one needs to do is to check whether this has been fixed
upstream and if so, to backport it. This is the case most of the time.
In the very seldom cases where a separate fix is needed, it needs to be
handled by asking Greg what to do. :-)

Adding stable@ folks to CC to set me straight if I'm missing something.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] acpi / apei: fix NULL deref during init

[Greg KH]

On Fri, Dec 14, 2018 at 11:54:28PM +0100, Borislav Petkov wrote:
> On Fri, Dec 14, 2018 at 11:42:01PM +0100, Thomas Schöbel-Theuer wrote:
> > Ah, I overlooked that commit e56c92565dfe2 is already providing a different
> > solution to the same problem in newer kernels _only_, as a _side_ effect
> > (not clear to me from the description, but clear from reading the code).
> 
> Damn, I missed the fact that this is not the upstream kernel:
> 
> CPU: 0 PID: 1 UID: 0 Comm: swapper/0 Not tainted 4.4.0-ui18344.004-uiabi1-infong-amd64 #1
> 
> > So another alternative would be backporting e56c92565dfe2 to the 4.4 LTS
> > series. Also fine for me.
> 
> That looks like the right fix.
> 
> A note for the next time: do not send a fix for a stable kernel which is
> not upstream:
> 
> >From Documentation/process/stable-kernel-rules.rst:
> 
> " - It or an equivalent fix must already exist in Linus' tree (upstream)."
> 
> The stable kernels track upstream so if a stable kernel has a problem,
> the first thing one needs to do is to check whether this has been fixed
> upstream and if so, to backport it. This is the case most of the time.
> In the very seldom cases where a separate fix is needed, it needs to be
> handled by asking Greg what to do. :-)
> 
> Adding stable@ folks to CC to set me straight if I'm missing something.

Nope, you are correct, thanks!

greg k-h