* [PATCH next 0/2] printk: fix reading beyond buffer
@ 2020-09-26 1:55 John Ogness
2020-09-26 1:55 ` [PATCH next 1/2] printk: avoid and/or handle record truncation John Ogness
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: John Ogness @ 2020-09-26 1:55 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Sergey Senozhatsky, Steven Rostedt,
Linus Torvalds, Greg Kroah-Hartman, Thomas Gleixner,
Marek Szyprowski, linux-kernel
Hello,
Marek Szyprowski reported [0] a problem with a particular printk
usage. This particular usage performs thousands of LOG_CONT calls.
The printk.c implementation was only limiting the growing record by
the maximum size available in the ringbuffer, thus creating a record
that was several kilobytes in size. This in and of itself is not
a problem.
However, the various readers used buffers that were about 1KB in
size. The ringbuffer would only fill the reader's 1KB buffer, but the
meta data stated that the message was actually much larger. The
reader code was not checking this and assumed its buffer contained
the full message.
I have solved this problem by adding the necessary check to the
functions where the situation can occur and also adding an argument
when extending records so that a maximum size is specified. This
will prevent the records from growing beyond the size that we know
our readers are using.
I did not add the check where it is certain that the reader's
buffer is large enough to contain the largest possible message.
The 2nd patch in this series reduces the size of the initial setup
buffer. I noticed it was too big while verifying all the sizes for
this series.
John Ogness
[0] https://lkml.kernel.org/r/f1651593-3579-5820-6863-5f4973d2bfdc@samsung.com
John Ogness (2):
printk: avoid and/or handle record truncation
printk: reduce setup_text_buf size to LOG_LINE_MAX
kernel/printk/printk.c | 9 +++++++--
kernel/printk/printk_ringbuffer.c | 12 ++++++++++--
kernel/printk/printk_ringbuffer.h | 2 +-
3 files changed, 18 insertions(+), 5 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH next 1/2] printk: avoid and/or handle record truncation 2020-09-26 1:55 [PATCH next 0/2] printk: fix reading beyond buffer John Ogness @ 2020-09-26 1:55 ` John Ogness 2020-09-28 6:23 ` Marek Szyprowski ` (2 more replies) 2020-09-26 1:55 ` [PATCH next 2/2] printk: reduce setup_text_buf size to LOG_LINE_MAX John Ogness 2020-09-26 3:28 ` [PATCH next 0/2] printk: fix reading beyond buffer Joe Perches 2 siblings, 3 replies; 8+ messages in thread From: John Ogness @ 2020-09-26 1:55 UTC (permalink / raw) To: Petr Mladek Cc: Sergey Senozhatsky, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds, Greg Kroah-Hartman, Thomas Gleixner, Marek Szyprowski, linux-kernel If a reader provides a buffer that is smaller than the message text, the @text_len field of @info will have a value larger than the buffer size. If readers blindly read @text_len bytes of data without checking the size, they will read beyond their buffer. Add this check to record_print_text() to properly recognize when such truncation needs to occur. Add a maximum size argument to the ringbuffer function to extend records so that records can not be created that are larger than the buffer size of readers. When extending records (LOG_CONT), do not extend records beyond LOG_LINE_MAX since that is the maximum size available in the buffers used by consoles and syslog. Fixes: f5f022e53b87 ("printk: reimplement log_cont using record extension") Signed-off-by: John Ogness <john.ogness@linutronix.de> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> --- kernel/printk/printk.c | 7 ++++++- kernel/printk/printk_ringbuffer.c | 12 ++++++++++-- kernel/printk/printk_ringbuffer.h | 2 +- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 78f68b4830dc..270f19b60e6f 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -1357,6 +1357,11 @@ static size_t record_print_text(struct printk_record *r, bool syslog, size_t len = 0; char *next; + if (text_len > buf_size) { + text_len = buf_size; + truncated = true; + } + prefix_len = info_print_prefix(r->info, syslog, time, prefix); /* @@ -1911,7 +1916,7 @@ static size_t log_output(int facility, int level, enum log_flags lflags, struct printk_record r; prb_rec_init_wr(&r, text_len); - if (prb_reserve_in_last(&e, prb, &r, caller_id)) { + if (prb_reserve_in_last(&e, prb, &r, caller_id, LOG_LINE_MAX)) { memcpy(&r.text_buf[r.info->text_len], text, text_len); r.info->text_len += text_len; if (lflags & LOG_NEWLINE) { diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c index 13b94b92342e..2493348a1631 100644 --- a/kernel/printk/printk_ringbuffer.c +++ b/kernel/printk/printk_ringbuffer.c @@ -202,7 +202,8 @@ * // specify additional 5 bytes text space to extend * prb_rec_init_wr(&r, 5); * - * if (prb_reserve_in_last(&e, &test_rb, &r, printk_caller_id())) { + * // try to extend, but only if it does not exceed 32 bytes + * if (prb_reserve_in_last(&e, &test_rb, &r, printk_caller_id()), 32) { * snprintf(&r.text_buf[r.info->text_len], * r.text_buf_size - r.info->text_len, "hello"); * @@ -1309,6 +1310,7 @@ static struct prb_desc *desc_reopen_last(struct prb_desc_ring *desc_ring, * @rb: The ringbuffer to re-reserve and extend data in. * @r: The record structure to allocate buffers for. * @caller_id: The caller ID of the caller (reserving writer). + * @max_size: Fail if the extended size would be greater than this. * * This is the public function available to writers to re-reserve and extend * data. @@ -1343,7 +1345,7 @@ static struct prb_desc *desc_reopen_last(struct prb_desc_ring *desc_ring, * @r->info->text_len after concatenating. */ bool prb_reserve_in_last(struct prb_reserved_entry *e, struct printk_ringbuffer *rb, - struct printk_record *r, u32 caller_id) + struct printk_record *r, u32 caller_id, unsigned int max_size) { struct prb_desc_ring *desc_ring = &rb->desc_ring; struct printk_info *info; @@ -1389,6 +1391,9 @@ bool prb_reserve_in_last(struct prb_reserved_entry *e, struct printk_ringbuffer if (!data_check_size(&rb->text_data_ring, r->text_buf_size)) goto fail; + if (r->text_buf_size > max_size) + goto fail; + r->text_buf = data_alloc(rb, &rb->text_data_ring, r->text_buf_size, &d->text_blk_lpos, id); } else { @@ -1410,6 +1415,9 @@ bool prb_reserve_in_last(struct prb_reserved_entry *e, struct printk_ringbuffer if (!data_check_size(&rb->text_data_ring, r->text_buf_size)) goto fail; + if (r->text_buf_size > max_size) + goto fail; + r->text_buf = data_realloc(rb, &rb->text_data_ring, r->text_buf_size, &d->text_blk_lpos, id); } diff --git a/kernel/printk/printk_ringbuffer.h b/kernel/printk/printk_ringbuffer.h index 0adaa685d1ca..5dc9d022db07 100644 --- a/kernel/printk/printk_ringbuffer.h +++ b/kernel/printk/printk_ringbuffer.h @@ -303,7 +303,7 @@ static inline void prb_rec_init_wr(struct printk_record *r, bool prb_reserve(struct prb_reserved_entry *e, struct printk_ringbuffer *rb, struct printk_record *r); bool prb_reserve_in_last(struct prb_reserved_entry *e, struct printk_ringbuffer *rb, - struct printk_record *r, u32 caller_id); + struct printk_record *r, u32 caller_id, unsigned int max_size); void prb_commit(struct prb_reserved_entry *e); void prb_final_commit(struct prb_reserved_entry *e); -- 2.20.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH next 1/2] printk: avoid and/or handle record truncation 2020-09-26 1:55 ` [PATCH next 1/2] printk: avoid and/or handle record truncation John Ogness @ 2020-09-28 6:23 ` Marek Szyprowski 2020-09-29 11:51 ` Petr Mladek 2020-09-29 13:04 ` Sergey Senozhatsky 2 siblings, 0 replies; 8+ messages in thread From: Marek Szyprowski @ 2020-09-28 6:23 UTC (permalink / raw) To: John Ogness, Petr Mladek Cc: Sergey Senozhatsky, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds, Greg Kroah-Hartman, Thomas Gleixner, linux-kernel Hi John, On 26.09.2020 03:55, John Ogness wrote: > If a reader provides a buffer that is smaller than the message text, > the @text_len field of @info will have a value larger than the buffer > size. If readers blindly read @text_len bytes of data without > checking the size, they will read beyond their buffer. > > Add this check to record_print_text() to properly recognize when such > truncation needs to occur. > > Add a maximum size argument to the ringbuffer function to extend > records so that records can not be created that are larger than the > buffer size of readers. > > When extending records (LOG_CONT), do not extend records beyond > LOG_LINE_MAX since that is the maximum size available in the buffers > used by consoles and syslog. > > Fixes: f5f022e53b87 ("printk: reimplement log_cont using record extension") > Signed-off-by: John Ogness <john.ogness@linutronix.de> > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> Thanks for fixing this issue! Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > kernel/printk/printk.c | 7 ++++++- > kernel/printk/printk_ringbuffer.c | 12 ++++++++++-- > kernel/printk/printk_ringbuffer.h | 2 +- > 3 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index 78f68b4830dc..270f19b60e6f 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -1357,6 +1357,11 @@ static size_t record_print_text(struct printk_record *r, bool syslog, > size_t len = 0; > char *next; > > + if (text_len > buf_size) { > + text_len = buf_size; > + truncated = true; > + } > + > prefix_len = info_print_prefix(r->info, syslog, time, prefix); > > /* > @@ -1911,7 +1916,7 @@ static size_t log_output(int facility, int level, enum log_flags lflags, > struct printk_record r; > > prb_rec_init_wr(&r, text_len); > - if (prb_reserve_in_last(&e, prb, &r, caller_id)) { > + if (prb_reserve_in_last(&e, prb, &r, caller_id, LOG_LINE_MAX)) { > memcpy(&r.text_buf[r.info->text_len], text, text_len); > r.info->text_len += text_len; > if (lflags & LOG_NEWLINE) { > diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c > index 13b94b92342e..2493348a1631 100644 > --- a/kernel/printk/printk_ringbuffer.c > +++ b/kernel/printk/printk_ringbuffer.c > @@ -202,7 +202,8 @@ > * // specify additional 5 bytes text space to extend > * prb_rec_init_wr(&r, 5); > * > - * if (prb_reserve_in_last(&e, &test_rb, &r, printk_caller_id())) { > + * // try to extend, but only if it does not exceed 32 bytes > + * if (prb_reserve_in_last(&e, &test_rb, &r, printk_caller_id()), 32) { > * snprintf(&r.text_buf[r.info->text_len], > * r.text_buf_size - r.info->text_len, "hello"); > * > @@ -1309,6 +1310,7 @@ static struct prb_desc *desc_reopen_last(struct prb_desc_ring *desc_ring, > * @rb: The ringbuffer to re-reserve and extend data in. > * @r: The record structure to allocate buffers for. > * @caller_id: The caller ID of the caller (reserving writer). > + * @max_size: Fail if the extended size would be greater than this. > * > * This is the public function available to writers to re-reserve and extend > * data. > @@ -1343,7 +1345,7 @@ static struct prb_desc *desc_reopen_last(struct prb_desc_ring *desc_ring, > * @r->info->text_len after concatenating. > */ > bool prb_reserve_in_last(struct prb_reserved_entry *e, struct printk_ringbuffer *rb, > - struct printk_record *r, u32 caller_id) > + struct printk_record *r, u32 caller_id, unsigned int max_size) > { > struct prb_desc_ring *desc_ring = &rb->desc_ring; > struct printk_info *info; > @@ -1389,6 +1391,9 @@ bool prb_reserve_in_last(struct prb_reserved_entry *e, struct printk_ringbuffer > if (!data_check_size(&rb->text_data_ring, r->text_buf_size)) > goto fail; > > + if (r->text_buf_size > max_size) > + goto fail; > + > r->text_buf = data_alloc(rb, &rb->text_data_ring, r->text_buf_size, > &d->text_blk_lpos, id); > } else { > @@ -1410,6 +1415,9 @@ bool prb_reserve_in_last(struct prb_reserved_entry *e, struct printk_ringbuffer > if (!data_check_size(&rb->text_data_ring, r->text_buf_size)) > goto fail; > > + if (r->text_buf_size > max_size) > + goto fail; > + > r->text_buf = data_realloc(rb, &rb->text_data_ring, r->text_buf_size, > &d->text_blk_lpos, id); > } > diff --git a/kernel/printk/printk_ringbuffer.h b/kernel/printk/printk_ringbuffer.h > index 0adaa685d1ca..5dc9d022db07 100644 > --- a/kernel/printk/printk_ringbuffer.h > +++ b/kernel/printk/printk_ringbuffer.h > @@ -303,7 +303,7 @@ static inline void prb_rec_init_wr(struct printk_record *r, > bool prb_reserve(struct prb_reserved_entry *e, struct printk_ringbuffer *rb, > struct printk_record *r); > bool prb_reserve_in_last(struct prb_reserved_entry *e, struct printk_ringbuffer *rb, > - struct printk_record *r, u32 caller_id); > + struct printk_record *r, u32 caller_id, unsigned int max_size); > void prb_commit(struct prb_reserved_entry *e); > void prb_final_commit(struct prb_reserved_entry *e); > Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH next 1/2] printk: avoid and/or handle record truncation 2020-09-26 1:55 ` [PATCH next 1/2] printk: avoid and/or handle record truncation John Ogness 2020-09-28 6:23 ` Marek Szyprowski @ 2020-09-29 11:51 ` Petr Mladek 2020-09-29 13:04 ` Sergey Senozhatsky 2 siblings, 0 replies; 8+ messages in thread From: Petr Mladek @ 2020-09-29 11:51 UTC (permalink / raw) To: John Ogness Cc: Sergey Senozhatsky, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds, Greg Kroah-Hartman, Thomas Gleixner, Marek Szyprowski, linux-kernel On Sat 2020-09-26 04:01:25, John Ogness wrote: > If a reader provides a buffer that is smaller than the message text, > the @text_len field of @info will have a value larger than the buffer > size. If readers blindly read @text_len bytes of data without > checking the size, they will read beyond their buffer. Great catch! > Add this check to record_print_text() to properly recognize when such > truncation needs to occur. > > Add a maximum size argument to the ringbuffer function to extend > records so that records can not be created that are larger than the > buffer size of readers. > > When extending records (LOG_CONT), do not extend records beyond > LOG_LINE_MAX since that is the maximum size available in the buffers > used by consoles and syslog. > > Fixes: f5f022e53b87 ("printk: reimplement log_cont using record extension") > Signed-off-by: John Ogness <john.ogness@linutronix.de> > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > kernel/printk/printk.c | 7 ++++++- > kernel/printk/printk_ringbuffer.c | 12 ++++++++++-- > kernel/printk/printk_ringbuffer.h | 2 +- > 3 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index 78f68b4830dc..270f19b60e6f 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -1357,6 +1357,11 @@ static size_t record_print_text(struct printk_record *r, bool syslog, > size_t len = 0; > char *next; > > + if (text_len > buf_size) { > + text_len = buf_size; > + truncated = true; @truncate must not be set here. Otherwise, the prefix would not be added when there no '\n' in the entire string. It would call: /* Drop truncated line(s). */ if (truncated) break; before copying the prefix. It is enough to remove the line. It will be set in the very first cycle anyway. We need to add one prefix at all. It would require to truncate even more bytes. Otherwise, the patch looks good to me. Best Regards, Petr ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH next 1/2] printk: avoid and/or handle record truncation 2020-09-26 1:55 ` [PATCH next 1/2] printk: avoid and/or handle record truncation John Ogness 2020-09-28 6:23 ` Marek Szyprowski 2020-09-29 11:51 ` Petr Mladek @ 2020-09-29 13:04 ` Sergey Senozhatsky 2 siblings, 0 replies; 8+ messages in thread From: Sergey Senozhatsky @ 2020-09-29 13:04 UTC (permalink / raw) To: John Ogness Cc: Petr Mladek, Sergey Senozhatsky, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds, Greg Kroah-Hartman, Thomas Gleixner, Marek Szyprowski, linux-kernel On (20/09/26 04:01), John Ogness wrote: > + if (text_len > buf_size) { > + text_len = buf_size; > + truncated = true; > + } > + > prefix_len = info_print_prefix(r->info, syslog, time, prefix); > > /* > @@ -1911,7 +1916,7 @@ static size_t log_output(int facility, int level, enum log_flags lflags, > struct printk_record r; > > prb_rec_init_wr(&r, text_len); > - if (prb_reserve_in_last(&e, prb, &r, caller_id)) { > + if (prb_reserve_in_last(&e, prb, &r, caller_id, LOG_LINE_MAX)) { Are we going to pass anything other than LOG_LINE_MAX? If not then maybe we can drop that argument and compare the text_buf_size to LOG_LINE_MAX? -ss ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH next 2/2] printk: reduce setup_text_buf size to LOG_LINE_MAX 2020-09-26 1:55 [PATCH next 0/2] printk: fix reading beyond buffer John Ogness 2020-09-26 1:55 ` [PATCH next 1/2] printk: avoid and/or handle record truncation John Ogness @ 2020-09-26 1:55 ` John Ogness 2020-09-29 11:55 ` Petr Mladek 2020-09-26 3:28 ` [PATCH next 0/2] printk: fix reading beyond buffer Joe Perches 2 siblings, 1 reply; 8+ messages in thread From: John Ogness @ 2020-09-26 1:55 UTC (permalink / raw) To: Petr Mladek Cc: Sergey Senozhatsky, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds, Greg Kroah-Hartman, Thomas Gleixner, Marek Szyprowski, linux-kernel @setup_text_buf only copies the original text messages (without any prefix or extended text). It only needs to be LOG_LINE_MAX in size. Signed-off-by: John Ogness <john.ogness@linutronix.de> --- kernel/printk/printk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 270f19b60e6f..c3eb97f93668 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -1103,7 +1103,7 @@ static unsigned int __init add_to_rb(struct printk_ringbuffer *rb, return prb_record_text_space(&e); } -static char setup_text_buf[CONSOLE_EXT_LOG_MAX] __initdata; +static char setup_text_buf[LOG_LINE_MAX] __initdata; void __init setup_log_buf(int early) { -- 2.20.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH next 2/2] printk: reduce setup_text_buf size to LOG_LINE_MAX 2020-09-26 1:55 ` [PATCH next 2/2] printk: reduce setup_text_buf size to LOG_LINE_MAX John Ogness @ 2020-09-29 11:55 ` Petr Mladek 0 siblings, 0 replies; 8+ messages in thread From: Petr Mladek @ 2020-09-29 11:55 UTC (permalink / raw) To: John Ogness Cc: Sergey Senozhatsky, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds, Greg Kroah-Hartman, Thomas Gleixner, Marek Szyprowski, linux-kernel On Sat 2020-09-26 04:01:26, John Ogness wrote: > @setup_text_buf only copies the original text messages (without any > prefix or extended text). It only needs to be LOG_LINE_MAX in size. > > Signed-off-by: John Ogness <john.ogness@linutronix.de> Reviewed-by: Petr Mladek <pmladek@suse.com> Best Regards, Petr ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH next 0/2] printk: fix reading beyond buffer 2020-09-26 1:55 [PATCH next 0/2] printk: fix reading beyond buffer John Ogness 2020-09-26 1:55 ` [PATCH next 1/2] printk: avoid and/or handle record truncation John Ogness 2020-09-26 1:55 ` [PATCH next 2/2] printk: reduce setup_text_buf size to LOG_LINE_MAX John Ogness @ 2020-09-26 3:28 ` Joe Perches 2 siblings, 0 replies; 8+ messages in thread From: Joe Perches @ 2020-09-26 3:28 UTC (permalink / raw) To: John Ogness, Petr Mladek Cc: Sergey Senozhatsky, Sergey Senozhatsky, Steven Rostedt, Linus Torvalds, Greg Kroah-Hartman, Thomas Gleixner, Marek Szyprowski, linux-kernel On Sat, 2020-09-26 at 04:01 +0206, John Ogness wrote: > Hello, > > Marek Szyprowski reported [0] a problem with a particular printk > usage. This particular usage performs thousands of LOG_CONT calls. > The printk.c implementation was only limiting the growing record by > the maximum size available in the ringbuffer, thus creating a record > that was several kilobytes in size. This in and of itself is not > a problem. Perhaps another mechanism would be to change the code to add a backspace before the rotor and have the printk ringbuffer actually backspace on \h when position > 0 Something like: --- diff --git a/init/do_mounts_rd.c b/init/do_mounts_rd.c index ac021ae6e6fa..8a36443b4866 100644 --- a/init/do_mounts_rd.c +++ b/init/do_mounts_rd.c @@ -257,7 +258,7 @@ int __init rd_load_image(char *from) kernel_write(out_file, buf, BLOCK_SIZE, &out_pos); #if !defined(CONFIG_S390) if (!(i % 16)) { - pr_cont("%c\b", rotator[rotate & 0x3]); + pr_cont("\h%c\b", rotator[rotate & 0x3]); rotate++; } #endif ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-09-29 13:04 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-26 1:55 [PATCH next 0/2] printk: fix reading beyond buffer John Ogness 2020-09-26 1:55 ` [PATCH next 1/2] printk: avoid and/or handle record truncation John Ogness 2020-09-28 6:23 ` Marek Szyprowski 2020-09-29 11:51 ` Petr Mladek 2020-09-29 13:04 ` Sergey Senozhatsky 2020-09-26 1:55 ` [PATCH next 2/2] printk: reduce setup_text_buf size to LOG_LINE_MAX John Ogness 2020-09-29 11:55 ` Petr Mladek 2020-09-26 3:28 ` [PATCH next 0/2] printk: fix reading beyond buffer Joe Perches
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.