Re: pstore/ram: printk: NULL characters in pstore ramoops area

[Kees Cook <keescook@chromium.org>]

On Thu, Aug 10, 2023 at 11:14:05AM +0200, Petr Mladek wrote:
> On Tue 2023-08-08 18:21:46, Vijay Balakrishna wrote:
> > Thanks for your reply Petr.
> > 
> > See inline.
> > 
> > On 8/8/23 01:15, Petr Mladek wrote:
> > > On Mon 2023-08-07 10:19:07, Vijay Balakrishna wrote:
> > > > I'm including my earlier email as it didn't deliver to
> > > > linux-kernel@vger.kernel.org due to HTML subpart.  Also sharing new findings
> > > > --
> > > > 
> > > > Limiting the size of buffer exposed to record_print_text() and
> > > > prb_for_each_record() in kmsg_dump_get_buffer() also resolves this issue [5]
> > > > -- no NULL characters in pstore/ramoops memory.  The advantage is no memory
> > > > allocation (as done in previously shared changes [4]) which could be
> > > > problematic during kernel shutdown/reboot or during kexec reboot.
> > > > 
> > > > [5]
> > > > 
> > > > Author: Vijay Balakrishna <vijayb@linux.microsoft.com>
> > > > Date:   Sat Aug 5 18:47:27 2023 +0000
> > > > 
> > > >      printk: limit the size of buffer exposed to record_print_text() by
> > > > kmsg_dump_get_buffer()
> > > > 
> > > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > > > index b82e4c3b52f4..8feec932aa35 100644
> > > > --- a/kernel/printk/printk.c
> > > > +++ b/kernel/printk/printk.c
> > > > @@ -3453,9 +3453,9 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper,
> > > > bool syslog,
> > > >           */
> > > >          next_seq = seq;
> > > > 
> > > > -       prb_rec_init_rd(&r, &info, buf, size);
> > > > 
> > > >          len = 0;
> > > > +       prb_rec_init_rd(&r, &info, buf + len, (size - len) >= LOG_LINE_MAX +
> > > > PREFIX_MAX ? LOG_LINE_MAX + PREFIX_MAX : size - len);
> > > >          prb_for_each_record(seq, prb, seq, &r) {
> > > >                  if (r.info->seq >= dumper->next_seq)
> > > >                          break;
> > > > @@ -3463,7 +3463,7 @@ bool kmsg_dump_get_buffer(struct kmsg_dumper *dumper,
> > > > bool syslog,
> > > >                  len += record_print_text(&r, syslog, time);
> > > > 
> > > >                  /* Adjust record to store to remaining buffer space. */
> > > > -               prb_rec_init_rd(&r, &info, buf + len, size - len);
> > > > +               prb_rec_init_rd(&r, &info, buf + len, (size - len) >=
> > > > LOG_LINE_MAX + PREFIX_MAX ? LOG_LINE_MAX + PREFIX_MAX : size - len);
> > > >          }
> > > > 
> > > >          dumper->next_seq = next_seq;
> > 
> > Any comments on above change to limit buffer size/range exposed?
> 
> I have the feeling that this is just a workaround. I would like to
> understand what exactly happens there. I want to be sure that
> there is no buffer overflow or other problems.
> 
> > The buffer passed to kmsg_dump_get_buffer() is kzalloc()'ed in
> > fs/pstore/ram.c: ramoops_probe()
> > 
> >                 cxt->pstore.buf = kzalloc(cxt->pstore.bufsize, GFP_KERNEL);
> > 
> > that may explain NULL characters in buffer.
> 
> Yeah, it might explain why there are so many '\0' in a row. Here is
> the dump from the initial mail:.

Okay, I think I'm caught up here in confirming what you've found
now that I'm able to reproduce it ("ramoops.record_size=0x80000
ramoops.max_reason=5"). Just for good measure (and to examine it
"externally") I disabled compression too ("pstore.compress=none").

If I do a "memset(dst, 'X', dst_size)" before calling
kmsg_dump_get_buffer(), the %NUL are now all 'X', so it's clear the kmsg
internals are skipping over bytes while writing: pstore makes a single
call to kmsg_dump_get_buffer() and performs no further buffer management
after this point.

On further investigation, I ultimately noticed the forced u16 cast for
buf_size in copy_data(). This was cast the wrong direction and any
buffer size larger than U16_MAX was getting wrapped/truncated. It should
be min_t for the larger type. I wonder how common this mistake is in the
kernel -- we should only ever GROW the type size when forcing a cast on
min_t() and max_t()...

This fixes it for me:


diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
index 2dc4d5a1f1ff..fde338606ce8 100644
--- a/kernel/printk/printk_ringbuffer.c
+++ b/kernel/printk/printk_ringbuffer.c
@@ -1735,7 +1735,7 @@ static bool copy_data(struct prb_data_ring *data_ring,
 	if (!buf || !buf_size)
 		return true;
 
-	data_size = min_t(u16, buf_size, len);
+	data_size = min_t(unsigned int, buf_size, len);
 
 	memcpy(&buf[0], data, data_size); /* LMM(copy_data:A) */
 	return true;

-- 
Kees Cook