On Tue, Jul 09, 2019 at 04:29:39PM +0200, Petr Mladek wrote:
> On Tue 2019-07-09 19:12:30, Sergey Senozhatsky wrote:
> > On (07/09/19 10:10), Vincent Whitchurch wrote:
> > > A dump of a 64-byte buffer filled by kmsg_dump_get_buffer(), before this
> > > patch:
> > >
> > > 00000000: 3c 30 3e 5b 20 20 20 20 36 2e 35 32 32 31 39 37 <0>[
> > > 6.522197
> > > 00000010: 5d 20 41 41 41 41 41 41 41 41 41 41 41 41 41 0a ]
> > > AAAAAAAAAAAAA.
> > > 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > ................
> > > 00000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > > ................
> > >
> > > After this patch:
> > >
> > > 00000000: 3c 30 3e 5b 20 20 20 20 36 2e 34 32 37 35 30 32 <0>[
> > > 6.427502
> > > 00000010: 5d 20 41 41 41 41 41 41 41 41 41 41 41 41 41 0a ]
> > > AAAAAAAAAAAAA.
> > > 00000020: 3c 30 3e 5b 20 20 20 20 36 2e 34 32 37 37 36 39 <0>[
> > > 6.427769
> > > 00000030: 5d 20 42 42 42 42 42 42 42 42 31 32 33 34 35 0a ]
> > > BBBBBBBB12345.
> >
> > [..]
> >
> > > @@ -1318,7 +1318,7 @@ static size_t msg_print_text(const struct
> > > printk_log *msg, bool syslog,
> > > }
> > >
> > > if (buf) {
> > > - if (prefix_len + text_len + 1 >= size - len)
> > > + if (prefix_len + text_len + 1 > size - len)
> > > break;
> >
> > So with this patch the last byte of the buffer is 0xA. It's a bit
> > uncomfortable that `len', which we return from msg_print_text(),
> > now points one byte beyond the buffer:
> >
> > buf[len++] = '\n';
> > return len;
> >
> > This is not very common. Not sure what usually happens to kmsg_dump
> > buffers, but anyone who'd do a rather innocent
> >
> > kmsg_dump(buf, &len);
> > buf[len] = 0x00;
> >
> > will write to something which is not a kmsg buffer (in some cases).
>
> I have the same worries.
>
> On the other hand. The function does not store the trailing '\0'
> into the buffer itself. The callers would need to add it themself.
> It is their responsibility to avoid a buffer overflow.
>
> I have checked several users and it seems that nobody adds or
> needs the trailing '\0'.
>
> It is ugly to do not use the entire buffer just because theoretical
> buggy users.
All the callers of kmsg_dump_get_buffer() seem to be fine but these two
users of kmsg_dump_get_line(), which also calls msg_print_text(), have
exactly the code which you and Sergey worry about:
arch/powerpc/xmon/xmon.c
2836: while (kmsg_dump_get_line_nolock(&dumper, false, buf, sizeof(buf),
&len)) {
2837- buf[len] = '\0';
arch/um/kernel/kmsg_dump.c
29: while (kmsg_dump_get_line(dumper, true, line, sizeof(line), &len)) {
30- line[len] = '\0';
I guess we should fix these first and leave this patch as is?
