Messages in this thread | | | Subject | Re: [PATCH next 1/2] printk: avoid and/or handle record truncation | From | Marek Szyprowski <> | Date | Mon, 28 Sep 2020 08:23:11 +0200 |
| |
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
| |