linux-kernel - Re: [PATCH printk v3 02/14] printk: Adjust mapping for 32bit seq macros

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87ttneed85.fsf@jogness.linutronix.de>
Date: Mon, 15 Jan 2024 18:14:26 +0106
From: John Ogness <john.ogness@...utronix.de>
To: Petr Mladek <pmladek@...e.com>
Cc: Sergey Senozhatsky <senozhatsky@...omium.org>, Steven Rostedt
 <rostedt@...dmis.org>, Thomas Gleixner <tglx@...utronix.de>,
 linux-kernel@...r.kernel.org, Francesco Dolcini <francesco@...cini.it>,
 kernel test robot <oliver.sang@...el.com>, Sebastian Andrzej Siewior
 <bigeasy@...utronix.de>
Subject: Re: [PATCH printk v3 02/14] printk: Adjust mapping for 32bit seq
 macros

On 2024-01-15, Petr Mladek <pmladek@...e.com> wrote:
> You know, the code around reading the messages is getting more
> and more complex. Also the u32 -> u64 transition is not 100% safe.
> So some sanity checks might be useful.

This complexity only exists for 32bit systems, but yeah, it is still
important.

> That said, I do not see any obvious trivial one. Just the following
> came to my mind. prb_first_seq() is reliable and we could do:
>
> void next_seq_sanity_check(struct printk_ringbuffer *rb, u64 next_seq)
> {
> 	struct prb_desc_ring *desc_ring = &rb->desc_ring;
> 	u64 first_seq = prb_first_seq(rb);
> 	u64 desc_count = DESC_COUNT(&rb->desc_ring)).
>
> 	WARN_ONCE(seq > first_seq + DESC_COUNT(desc_ring));
> }

@seq is allowed to be 2^31 before or after @first_seq. The check would
look more like this:

    WARN_ONCE((rb_first_seq < 0xffffffff80000000 &&
               seq > rb_first_seq + 0x80000000) ||
              (rb_first_seq > 0x80000000 &&
               seq < rb_first_seq - 0x80000000));

> Well, I am not sure if it is worth it. Also the WARN() won't be
> printed on consoles when the reading is broken.

Broken printing is irrelevant. There are plenty of debug methods to get
at the ringbuffer. I am OK with adding this sanity check (again, only
for 32bit, within __ulseq_to_u64seq()).

John