[PATCH next 0/2] printk: fix reading beyond buffer

[PATCH next 0/2] printk: fix reading beyond buffer

[John Ogness]

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

[PATCH next 1/2] printk: avoid and/or handle record truncation

[John Ogness]

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

[PATCH next 2/2] printk: reduce setup_text_buf size to LOG_LINE_MAX

[John Ogness]

@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

Re: [PATCH next 0/2] printk: fix reading beyond buffer

[Joe Perches]

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

Re: [PATCH next 1/2] printk: avoid and/or handle record truncation

[Marek Szyprowski]

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

Re: [PATCH next 1/2] printk: avoid and/or handle record truncation

[Petr Mladek]

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

Re: [PATCH next 2/2] printk: reduce setup_text_buf size to LOG_LINE_MAX

[Petr Mladek]

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

Re: [PATCH next 1/2] printk: avoid and/or handle record truncation

[Sergey Senozhatsky]

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