linux-kernel - Re: [BUG] kernel/printk: RT-adaption of kmsg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <87zgsb4eot.fsf@jogness.linutronix.de>
Date:   Fri, 17 Sep 2021 13:42:02 +0206
From:   John Ogness <john.ogness@...utronix.de>
To:     Walter Stoll <Walter.Stoll@...gon.com>,
        "linux-rt-users@...r.kernel.org" <linux-rt-users@...r.kernel.org>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [BUG] kernel/printk: RT-adaption of kmsg_dump breaks mtdoops

Hello Walter,

On 2021-09-17, Walter Stoll <Walter.Stoll@...gon.com> wrote:
> Effect observed
> ---------------
>
> We want to store kernel oops logs on a NAND flash partition by means of the
> MTD Oops/Panic console logger/driver. However, no log is generated.
>
> We observed the effect with kernel version v5.4.138-rt62. However, we think
> that the most recent kernel exhibits the same behavior because the structure of
> the sources in question (see below) did not change.

This problem does not exist in v5.10-rt. The kmsg_dump API was changed
(later in mainline as well) to avoid this issue. For v5.10-rt the change
was buried in the all-in-one commit:

https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-stable-rt.git/commit/?h=v5.10-rt&id=9835363e31e75d3287a81c6fc1b22c5c2b43d277

And later. the API change for mainline (5.12):

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f9f3f02db98bbe678a8e57fe9432b196174744a3

> Root cause
> ----------
>
> Further investigations revealed, than the issue is due to the realtime adaption
> of the kmsg_dump() function.
>
> Non realtime version of kmsg_dump():
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/kernel/printk/printk.c?h=linux-5.4.y#n3149
>
> Realtime version of kmsg_dump():
> https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-stable-rt.git/tree/kernel/printk/printk.c?h=v5.4-rt#n2904
>
> In our case, the the mtd_panic_write() function returns with error when the
> kernel panics. As a result, no log is created. Call chain:
> ... panic() -> kmsg_dump() -> mtdoops_do_dump() -> mtdoops_write()
>     mtd_panic_write()
>
> Note that within the kmsg_dump() function, a locally create kmsg_dumper object
> is passed to the dumper_local.dump(&dumper_local, reason) call. However the
> callee mtdoops_do_dump() function expects this object being embedded in a
> mtdoops_context object, see
> https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-stable-rt.git/tree/drivers/mtd/mtdoops.c?h=v5.4-rt#n272
>
> This is obviously not the case, therefore the error. Note that this could even
> led to a security issue.
>
>
> Bug fix proposal
> ----------------
>
> We fixed the bug locally by applying the patch below. Note that we did not
> modify the kmsg_dump() function. Instead we modified the mtdoops_do_dump()
> function which seems for us the cleaner way.

I agree that this is a better fix, rather than backporting the newer
API. For mtdoops, the only function registering a dumper is
mtdoops_notify_add() and it also directly references the global context
with:

struct mtdoops_context *cxt = &oops_cxt;

Reviewed-by: John Ogness <john.ogness@...utronix.de>

> diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
> index 774970bfcf85..317ecf47fce8 100644
> --- a/drivers/mtd/mtdoops.c
> +++ b/drivers/mtd/mtdoops.c
> @@ -269,8 +269,7 @@ static void find_next_position(struct mtdoops_context *cxt)
>  static void mtdoops_do_dump(struct kmsg_dumper *dumper,
>  			    enum kmsg_dump_reason reason)
>  {
> -	struct mtdoops_context *cxt = container_of(dumper,
> -			struct mtdoops_context, dump);
> +	struct mtdoops_context *cxt = &oops_cxt;
>  
>  	/* Only dump oopses if dump_oops is set */
>  	if (reason == KMSG_DUMP_OOPS && !dump_oops)

Thanks for your work on this!

John Ogness