From: "Steven Rostedt (Red Hat)" <[email protected]>
Currently seq_buf is full when all but one byte of the buffer is
filled. Change it so that the seq_buf is full when all of the
buffer is filled.
Some of the functions would fill the buffer completely and report
everything was fine. This was inconsistent with the max of size - 1.
Changing this to be max of size makes all functions consistent.
Signed-off-by: Steven Rostedt <[email protected]>
---
include/linux/seq_buf.h | 4 ++--
kernel/trace/seq_buf.c | 18 +++++++++---------
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
index 064a8604ad33..3cd25038cb5e 100644
--- a/include/linux/seq_buf.h
+++ b/include/linux/seq_buf.h
@@ -46,13 +46,13 @@ seq_buf_init(struct seq_buf *s, unsigned char *buf, unsigned int size)
static inline bool
seq_buf_has_overflowed(struct seq_buf *s)
{
- return s->len == s->size;
+ return s->len > s->size;
}
static inline void
seq_buf_set_overflow(struct seq_buf *s)
{
- s->len = s->size;
+ s->len = s->size + 1;
}
extern __printf(2, 3)
diff --git a/kernel/trace/seq_buf.c b/kernel/trace/seq_buf.c
index 243123b12d16..06fd1833e692 100644
--- a/kernel/trace/seq_buf.c
+++ b/kernel/trace/seq_buf.c
@@ -11,17 +11,17 @@
* This will set up the counters within the descriptor. You can call
* seq_buf_init() more than once to reset the seq_buf to start
* from scratch.
- *
+ *
*/
#include <linux/uaccess.h>
#include <linux/seq_file.h>
#include <linux/seq_buf.h>
/* How much buffer is left on the seq_buf? */
-#define SEQ_BUF_LEFT(s) (((s)->size - 1) - (s)->len)
+#define SEQ_BUF_LEFT(s) ((s)->size - (s)->len)
/* How much buffer is written? */
-#define SEQ_BUF_USED(s) min((s)->len, (s)->size - 1)
+#define SEQ_BUF_USED(s) min((s)->len, (s)->size)
/**
* seq_buf_print_seq - move the contents of seq_buf into a seq_file
@@ -55,7 +55,7 @@ int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args)
if (s->len < s->size) {
len = vsnprintf(s->buffer + s->len, s->size - s->len, fmt, args);
- if (s->len + len < s->size) {
+ if (s->len + len <= s->size) {
s->len += len;
return 0;
}
@@ -105,7 +105,7 @@ int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
if (s->len < s->size) {
ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);
- if (s->len + ret < s->size) {
+ if (s->len + ret <= s->size) {
s->len += ret;
return 0;
}
@@ -140,7 +140,7 @@ int seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary)
if (s->len < s->size) {
ret = bstr_printf(s->buffer + s->len, len, fmt, binary);
- if (s->len + ret < s->size) {
+ if (s->len + ret <= s->size) {
s->len += ret;
return 0;
}
@@ -164,7 +164,7 @@ int seq_buf_puts(struct seq_buf *s, const char *str)
WARN_ON(s->size == 0);
- if (s->len + len < s->size) {
+ if (s->len + len <= s->size) {
memcpy(s->buffer + s->len, str, len);
s->len += len;
return 0;
@@ -186,7 +186,7 @@ int seq_buf_putc(struct seq_buf *s, unsigned char c)
{
WARN_ON(s->size == 0);
- if (s->len + 1 < s->size) {
+ if (s->len + 1 <= s->size) {
s->buffer[s->len++] = c;
return 0;
}
@@ -210,7 +210,7 @@ int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len)
{
WARN_ON(s->size == 0);
- if (s->len + len < s->size) {
+ if (s->len + len <= s->size) {
memcpy(s->buffer + s->len, mem, len);
s->len += len;
return 0;
--
2.1.1
On Tue 2014-11-04 10:52:44, Steven Rostedt wrote:
> From: "Steven Rostedt (Red Hat)" <[email protected]>
>
> Currently seq_buf is full when all but one byte of the buffer is
> filled. Change it so that the seq_buf is full when all of the
> buffer is filled.
>
> Some of the functions would fill the buffer completely and report
> everything was fine. This was inconsistent with the max of size - 1.
> Changing this to be max of size makes all functions consistent.
>
> Signed-off-by: Steven Rostedt <[email protected]>
> ---
> include/linux/seq_buf.h | 4 ++--
> kernel/trace/seq_buf.c | 18 +++++++++---------
> 2 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
> index 064a8604ad33..3cd25038cb5e 100644
> --- a/include/linux/seq_buf.h
> +++ b/include/linux/seq_buf.h
> @@ -46,13 +46,13 @@ seq_buf_init(struct seq_buf *s, unsigned char *buf, unsigned int size)
> static inline bool
> seq_buf_has_overflowed(struct seq_buf *s)
> {
> - return s->len == s->size;
> + return s->len > s->size;
> }
>
> static inline void
> seq_buf_set_overflow(struct seq_buf *s)
> {
> - s->len = s->size;
> + s->len = s->size + 1;
> }
>
> extern __printf(2, 3)
> diff --git a/kernel/trace/seq_buf.c b/kernel/trace/seq_buf.c
> index 243123b12d16..06fd1833e692 100644
> --- a/kernel/trace/seq_buf.c
> +++ b/kernel/trace/seq_buf.c
> @@ -11,17 +11,17 @@
> * This will set up the counters within the descriptor. You can call
> * seq_buf_init() more than once to reset the seq_buf to start
> * from scratch.
> - *
> + *
> */
> #include <linux/uaccess.h>
> #include <linux/seq_file.h>
> #include <linux/seq_buf.h>
>
> /* How much buffer is left on the seq_buf? */
> -#define SEQ_BUF_LEFT(s) (((s)->size - 1) - (s)->len)
> +#define SEQ_BUF_LEFT(s) ((s)->size - (s)->len)
>
> /* How much buffer is written? */
> -#define SEQ_BUF_USED(s) min((s)->len, (s)->size - 1)
> +#define SEQ_BUF_USED(s) min((s)->len, (s)->size)
>
> /**
> * seq_buf_print_seq - move the contents of seq_buf into a seq_file
> @@ -55,7 +55,7 @@ int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args)
>
> if (s->len < s->size) {
> len = vsnprintf(s->buffer + s->len, s->size - s->len, fmt, args);
> - if (s->len + len < s->size) {
> + if (s->len + len <= s->size) {
This is always true because we limit vsnprintf() to write (s->size -
s->len) bytes. Similar problem is also in the other parts of this
patch.
I wonder if we want this change at all. It means that we are not able to
detect overflow in some functions. It is pity because the users
might want to increase the buffer size and try again if the print
was incomplete.
I think that we need to leave the one byte for the overflow detection
if we want to detect it properly.
Best Regards,
Petr
> s->len += len;
> return 0;
> }
> @@ -105,7 +105,7 @@ int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
>
> if (s->len < s->size) {
> ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);
> - if (s->len + ret < s->size) {
> + if (s->len + ret <= s->size) {
> s->len += ret;
> return 0;
> }
On Wed, 5 Nov 2014 17:31:50 +0100
Petr Mladek <[email protected]> wrote:
> > /**
> > * seq_buf_print_seq - move the contents of seq_buf into a seq_file
> > @@ -55,7 +55,7 @@ int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args)
> >
> > if (s->len < s->size) {
> > len = vsnprintf(s->buffer + s->len, s->size - s->len, fmt, args);
> > - if (s->len + len < s->size) {
> > + if (s->len + len <= s->size) {
>
> This is always true because we limit vsnprintf() to write (s->size -
> s->len) bytes. Similar problem is also in the other parts of this
> patch.
No, len is the length of bytes that should have been written, not the
amount that has been written.
>
> I wonder if we want this change at all. It means that we are not able to
> detect overflow in some functions. It is pity because the users
> might want to increase the buffer size and try again if the print
> was incomplete.
What do you mean we can't detect overflow? That's what
seq_buf_has_overflowed() does.
>
> I think that we need to leave the one byte for the overflow detection
> if we want to detect it properly.
I don't.
-- Steve
On Wed, 5 Nov 2014 15:21:30 -0500
Steven Rostedt <[email protected]> wrote:
> >
> > I wonder if we want this change at all. It means that we are not able to
> > detect overflow in some functions. It is pity because the users
> > might want to increase the buffer size and try again if the print
> > was incomplete.
>
> What do you mean we can't detect overflow? That's what
> seq_buf_has_overflowed() does.
>
Although I'm looking at the seq_file versions of the bitmap code, which
does only return the len of what was written and not what would have
been written, and it does have this issue.
I hate to go back to the -1 of the size of buffer as that causes
inconsistencies within the functions themselves, as proved with the
seq_file code.
What I might do as just have the bitmap calls not be allowed to fill
the buffer and keep the logic the same. That is, if the bitmap calls
fill the rest of the length, assume we overflowed, otherwise we are
fine.
I'm going to change seq_buf to do that instead of my new update with
the bitmask code.
-- Steve
On Wed 2014-11-05 15:21:30, Steven Rostedt wrote:
> On Wed, 5 Nov 2014 17:31:50 +0100
> Petr Mladek <[email protected]> wrote:
>
>
> > > /**
> > > * seq_buf_print_seq - move the contents of seq_buf into a seq_file
> > > @@ -55,7 +55,7 @@ int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args)
> > >
> > > if (s->len < s->size) {
> > > len = vsnprintf(s->buffer + s->len, s->size - s->len, fmt, args);
> > > - if (s->len + len < s->size) {
> > > + if (s->len + len <= s->size) {
> >
> > This is always true because we limit vsnprintf() to write (s->size -
> > s->len) bytes. Similar problem is also in the other parts of this
> > patch.
>
> No, len is the length of bytes that should have been written, not the
> amount that has been written.
That is cool! I did not know that there was also this variant of the
printf() functions. I am still learning.
Best Regards,
Petr
On Wed 2014-11-05 16:06:18, Steven Rostedt wrote:
> On Wed, 5 Nov 2014 15:21:30 -0500
> Steven Rostedt <[email protected]> wrote:
>
> > >
> > > I wonder if we want this change at all. It means that we are not able to
> > > detect overflow in some functions. It is pity because the users
> > > might want to increase the buffer size and try again if the print
> > > was incomplete.
> >
> > What do you mean we can't detect overflow? That's what
> > seq_buf_has_overflowed() does.
> >
>
> Although I'm looking at the seq_file versions of the bitmap code, which
> does only return the len of what was written and not what would have
> been written, and it does have this issue.
>
> I hate to go back to the -1 of the size of buffer as that causes
> inconsistencies within the functions themselves, as proved with the
> seq_file code.
Yeah, the -1 and the unused byte is strange and it would be great to
avoid it.
On the other hand, I am slightly afraid of the "len = size + 1" that
signalizes the buffer overflow. It might be prone for creating security
bugs. If people forget to check seq_buf_has_overflowed() before
reading or if there is a race, they might read outside of the buffer.
> What I might do as just have the bitmap calls not be allowed to fill
> the buffer and keep the logic the same. That is, if the bitmap calls
> fill the rest of the length, assume we overflowed, otherwise we are
> fine.
>
> I'm going to change seq_buf to do that instead of my new update with
> the bitmask code.
I like the idea of having the exception only in the bitmap code and filling
the whole buffer in other cases.
I am now in doubts about the overflow state. A solution would
be to add an "overflow" flag to struct seq_bug. I agree that it
is ugly but it looks more secure then "len = size + 1".
Well, I do not have that strong opinion about it. What do you think?
Best Regards,
Petr
On Thu, 6 Nov 2014 16:31:07 +0100
Petr Mladek <[email protected]> wrote:
> I like the idea of having the exception only in the bitmap code and filling
> the whole buffer in other cases.
>
> I am now in doubts about the overflow state. A solution would
> be to add an "overflow" flag to struct seq_bug. I agree that it
> is ugly but it looks more secure then "len = size + 1".
>
> Well, I do not have that strong opinion about it. What do you think?
Ideally, I want struct seq_buf defined only within seq_buf.c, and all
users must access the buffer via function methods.
We would need to remove all the inline calls, which will have a small
affect on performance. But most seq_buf code is for output of text,
where this overhead would be a nit to the total cost of operations.
That would get rid of all users that think it's safe to access
s->buffer + s->len.
-- Steve
On Thu 2014-11-06 14:24:02, Steven Rostedt wrote:
> On Thu, 6 Nov 2014 16:31:07 +0100
> Petr Mladek <[email protected]> wrote:
>
>
> > I like the idea of having the exception only in the bitmap code and filling
> > the whole buffer in other cases.
> >
> > I am now in doubts about the overflow state. A solution would
> > be to add an "overflow" flag to struct seq_bug. I agree that it
> > is ugly but it looks more secure then "len = size + 1".
> >
> > Well, I do not have that strong opinion about it. What do you think?
>
> Ideally, I want struct seq_buf defined only within seq_buf.c, and all
> users must access the buffer via function methods.
>
> We would need to remove all the inline calls, which will have a small
> affect on performance. But most seq_buf code is for output of text,
> where this overhead would be a nit to the total cost of operations.
>
> That would get rid of all users that think it's safe to access
> s->buffer + s->len.
Sounds like a good plan.
Best Regards,
Petr
Some more touch ups.
-- Steve
>From 7e724556c21178a9890b31ff57f69761b41f435a Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (Red Hat)" <[email protected]>
Date: Wed, 29 Oct 2014 15:26:09 -0400
Subject: [PATCH] tracing: Have seq_buf use full buffer
Currently seq_buf is full when all but one byte of the buffer is
filled. Change it so that the seq_buf is full when all of the
buffer is filled.
Some of the functions would fill the buffer completely and report
everything was fine. This was inconsistent with the max of size - 1.
Changing this to be max of size makes all functions consistent.
Link: http://lkml.kernel.org/r/[email protected]
Tested-by: Jiri Kosina <[email protected]>
Acked-by: Jiri Kosina <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
---
include/linux/seq_buf.h | 6 +++---
kernel/trace/seq_buf.c | 19 +++++++++++--------
2 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
index f68fde850ef7..4aab47d10760 100644
--- a/include/linux/seq_buf.h
+++ b/include/linux/seq_buf.h
@@ -43,13 +43,13 @@ seq_buf_init(struct seq_buf *s, unsigned char *buf, unsigned int size)
static inline bool
seq_buf_has_overflowed(struct seq_buf *s)
{
- return s->len == s->size;
+ return s->len > s->size;
}
static inline void
seq_buf_set_overflow(struct seq_buf *s)
{
- s->len = s->size;
+ s->len = s->size + 1;
}
/*
@@ -58,7 +58,7 @@ seq_buf_set_overflow(struct seq_buf *s)
static inline unsigned int
seq_buf_buffer_left(struct seq_buf *s)
{
- return (s->size - 1) - s->len;
+ return s->size - s->len;
}
extern __printf(2, 3)
diff --git a/kernel/trace/seq_buf.c b/kernel/trace/seq_buf.c
index ac6eb864c946..8e60b5c1b9f3 100644
--- a/kernel/trace/seq_buf.c
+++ b/kernel/trace/seq_buf.c
@@ -17,7 +17,7 @@
#include <linux/seq_buf.h>
/* How much buffer is written? */
-#define SEQ_BUF_USED(s) min((s)->len, (s)->size - 1)
+#define SEQ_BUF_USED(s) min((s)->len, (s)->size)
/**
* seq_buf_print_seq - move the contents of seq_buf into a seq_file
@@ -51,7 +51,7 @@ int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args)
if (s->len < s->size) {
len = vsnprintf(s->buffer + s->len, s->size - s->len, fmt, args);
- if (s->len + len < s->size) {
+ if (s->len + len <= s->size) {
s->len += len;
return 0;
}
@@ -100,8 +100,11 @@ int seq_buf_bitmask(struct seq_buf *s, const unsigned long *maskp,
WARN_ON(s->size == 0);
/*
- * The last byte of the buffer is used to determine if we
- * overflowed or not.
+ * Note, because bitmap_scnprintf() only returns the number of bytes
+ * written and not the number that would be written, we use the last
+ * byte of the buffer to let us know if we overflowed. There's a small
+ * chance that the bitmap could have fit exactly inside the buffer, but
+ * it's not that critical if that does happen.
*/
if (len > 1) {
ret = bitmap_scnprintf(s->buffer, len, maskp, nmaskbits);
@@ -140,7 +143,7 @@ int seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary)
if (s->len < s->size) {
ret = bstr_printf(s->buffer + s->len, len, fmt, binary);
- if (s->len + ret < s->size) {
+ if (s->len + ret <= s->size) {
s->len += ret;
return 0;
}
@@ -164,7 +167,7 @@ int seq_buf_puts(struct seq_buf *s, const char *str)
WARN_ON(s->size == 0);
- if (s->len + len < s->size) {
+ if (s->len + len <= s->size) {
memcpy(s->buffer + s->len, str, len);
s->len += len;
return 0;
@@ -186,7 +189,7 @@ int seq_buf_putc(struct seq_buf *s, unsigned char c)
{
WARN_ON(s->size == 0);
- if (s->len + 1 < s->size) {
+ if (s->len + 1 <= s->size) {
s->buffer[s->len++] = c;
return 0;
}
@@ -210,7 +213,7 @@ int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len)
{
WARN_ON(s->size == 0);
- if (s->len + len < s->size) {
+ if (s->len + len <= s->size) {
memcpy(s->buffer + s->len, mem, len);
s->len += len;
return 0;
--
2.1.1
On Fri 2014-11-07 13:37:29, Steven Rostedt wrote:
>
> Some more touch ups.
>
> -- Steve
>
> From 7e724556c21178a9890b31ff57f69761b41f435a Mon Sep 17 00:00:00 2001
> From: "Steven Rostedt (Red Hat)" <[email protected]>
> Date: Wed, 29 Oct 2014 15:26:09 -0400
> Subject: [PATCH] tracing: Have seq_buf use full buffer
>
> Currently seq_buf is full when all but one byte of the buffer is
> filled. Change it so that the seq_buf is full when all of the
> buffer is filled.
>
> Some of the functions would fill the buffer completely and report
> everything was fine. This was inconsistent with the max of size - 1.
> Changing this to be max of size makes all functions consistent.
>
> Link: http://lkml.kernel.org/r/[email protected]
>
> Tested-by: Jiri Kosina <[email protected]>
> Acked-by: Jiri Kosina <[email protected]>
> Signed-off-by: Steven Rostedt <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
This patch might need a refresh if you add the extra buffer overflow
check into seq_buf_set_overflow() in the 3rd patch. Anyway, changes
in this patch looks good.
I am happy with all the changes in the patchset. Thanks a lot for
driving it forward.
Best Regards,
Petr