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 <[email protected]>
Reported-by: Marek Szyprowski <[email protected]>
---
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
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 <[email protected]>
> Reported-by: Marek Szyprowski <[email protected]>
Thanks for fixing this issue!
Tested-by: Marek Szyprowski <[email protected]>
> ---
> 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
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 <[email protected]>
> Reported-by: Marek Szyprowski <[email protected]>
> ---
> 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
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