Messages in this thread Patch in this message | | | Date | Thu, 10 Aug 2023 22:23:48 -0700 | From | Kees Cook <> | Subject | Re: pstore/ram: printk: NULL characters in pstore ramoops area |
| |
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
| |