Messages in this thread | | | Date | Wed, 10 Jul 2019 10:04:02 +0200 | From | Vincent Whitchurch <> | Subject | Re: [PATCH] printk: Do not lose last line in kmsg dump |
| |
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?
| |