LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [GIT PULL] seq_buf: Fix seq_buf_vprintf() truncation
@ 2015-03-04 20:12 Steven Rostedt
  2015-03-04 20:21 ` Joe Perches
  0 siblings, 1 reply; 3+ messages in thread
From: Steven Rostedt @ 2015-03-04 20:12 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: LKML, Ingo Molnar, Andrew Morton


Linus,

While working on merging seq_file and seq_buf code, I hit a bug and came
down to a bug in seq_buf_printf(). This is suppose to be a all or nothing
function. That is, either all of the string passed in is saved to the
seq_buf buffer, or none of it. But due to the way vsnprintf() works in
the kernel, if the line is truncated, the return value is the amount
written to the buffer, including the '\0'. Then the test in seq_buf_printf()
uses the returned length to see if it would fit in the buffer. It will,
but it will also fill the buffer. The test to see if vsnprintf() overflowed
or not is to see if the length returned is equal to or greater than
the length of the buffer passed to it. Not just greater than.

Please pull the latest trace-fixes-v4.0-rc2 tree, which can be found at:


  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
trace-fixes-v4.0-rc2

Tag SHA1: 2edf55a81800ad8200a02de31c8099edbdbd283f
Head SHA1: 4a8fe4e1811c96ad0ad9f4083f2fe4fb43b2988d


Steven Rostedt (Red Hat) (1):
      seq_buf: Fix seq_buf_vprintf() truncation

----
 lib/seq_buf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
---------------------------
commit 4a8fe4e1811c96ad0ad9f4083f2fe4fb43b2988d
Author: Steven Rostedt (Red Hat) <rostedt@goodmis.org>
Date:   Wed Mar 4 09:56:02 2015 -0500

    seq_buf: Fix seq_buf_vprintf() truncation
    
    In seq_buf_vprintf(), vsnprintf() is used to copy the format into the
    buffer remaining in the seq_buf structure. The return of vsnprintf()
    is the amount of characters written to the buffer excluding the '\0',
    unless the line was truncated!
    
    If the line copied does not fit, it is truncated, and a '\0' is added
    to the end of the buffer. But in this case, '\0' is included in the length
    of the line written. To know if the buffer had overflowed, the return
    length will be the same as the length of the buffer passed in.
    
    The check in seq_buf_vprintf() only checked if the length returned from
    vsnprintf() would fit in the buffer, as the seq_buf_vprintf() is only
    to be an all or nothing command. It either writes all the string into
    the seq_buf, or none of it. If the string is truncated, the pointers
    inside the seq_buf must be reset to what they were when the function was
    called. This is not the case. On overflow, it copies only part of the string.
    
    The fix is to change the overflow check to see if the length returned from
    vsnprintf() is less than the length remaining in the seq_buf buffer, and not
    if it is less than or equal to as it currently does. Then seq_buf_vprintf()
    will know if the write from vsnpritnf() was truncated or not.
    
    Cc: stable@vger.kernel.org
    Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/lib/seq_buf.c b/lib/seq_buf.c
index 88c0854bd752..0c92583b7b7e 100644
--- a/lib/seq_buf.c
+++ b/lib/seq_buf.c
@@ -61,7 +61,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 (seq_buf_can_fit(s, len)) {
+		if (s->len + len < s->size) {
 			s->len += len;
 			return 0;
 		}

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [GIT PULL] seq_buf: Fix seq_buf_vprintf() truncation
  2015-03-04 20:12 [GIT PULL] seq_buf: Fix seq_buf_vprintf() truncation Steven Rostedt
@ 2015-03-04 20:21 ` Joe Perches
  2015-03-04 20:29   ` Steven Rostedt
  0 siblings, 1 reply; 3+ messages in thread
From: Joe Perches @ 2015-03-04 20:21 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Linus Torvalds, LKML, Ingo Molnar, Andrew Morton

On Wed, 2015-03-04 at 15:12 -0500, Steven Rostedt wrote:
> Linus,
> 
> While working on merging seq_file and seq_buf code, I hit a bug and came
> down to a bug in seq_buf_printf(). This is suppose to be a all or nothing
> function. That is, either all of the string passed in is saved to the
> seq_buf buffer, or none of it. But due to the way vsnprintf() works in
> the kernel, if the line is truncated, the return value is the amount
> written to the buffer, including the '\0'. Then the test in seq_buf_printf()
> uses the returned length to see if it would fit in the buffer. It will,
> but it will also fill the buffer. The test to see if vsnprintf() overflowed
> or not is to see if the length returned is equal to or greater than
> the length of the buffer passed to it. Not just greater than.
> 
> Please pull the latest trace-fixes-v4.0-rc2 tree, which can be found at:
[]
> commit 4a8fe4e1811c96ad0ad9f4083f2fe4fb43b2988d
> Author: Steven Rostedt (Red Hat) <rostedt@goodmis.org>
> Date:   Wed Mar 4 09:56:02 2015 -0500
> 
>     seq_buf: Fix seq_buf_vprintf() truncation
>     
>     In seq_buf_vprintf(), vsnprintf() is used to copy the format into the
>     buffer remaining in the seq_buf structure. The return of vsnprintf()
>     is the amount of characters written to the buffer excluding the '\0',
>     unless the line was truncated!
>     
>     If the line copied does not fit, it is truncated, and a '\0' is added
>     to the end of the buffer. But in this case, '\0' is included in the length
>     of the line written. To know if the buffer had overflowed, the return
>     length will be the same as the length of the buffer passed in.
>     
>     The check in seq_buf_vprintf() only checked if the length returned from
>     vsnprintf() would fit in the buffer, as the seq_buf_vprintf() is only
>     to be an all or nothing command. It either writes all the string into
>     the seq_buf, or none of it. If the string is truncated, the pointers
>     inside the seq_buf must be reset to what they were when the function was
>     called. This is not the case. On overflow, it copies only part of the string.
>     
>     The fix is to change the overflow check to see if the length returned from
>     vsnprintf() is less than the length remaining in the seq_buf buffer, and not
>     if it is less than or equal to as it currently does. Then seq_buf_vprintf()
>     will know if the write from vsnpritnf() was truncated or not.
[]
> diff --git a/lib/seq_buf.c b/lib/seq_buf.c
[]
> @@ -61,7 +61,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);

Presumably the seq_buf_bprintf function has the same issue.



^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [GIT PULL] seq_buf: Fix seq_buf_vprintf() truncation
  2015-03-04 20:21 ` Joe Perches
@ 2015-03-04 20:29   ` Steven Rostedt
  0 siblings, 0 replies; 3+ messages in thread
From: Steven Rostedt @ 2015-03-04 20:29 UTC (permalink / raw)
  To: Joe Perches; +Cc: Linus Torvalds, LKML, Ingo Molnar, Andrew Morton

On Wed, 04 Mar 2015 12:21:21 -0800
Joe Perches <joe@perches.com> wrote:


> > @@ -61,7 +61,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);
> 
> Presumably the seq_buf_bprintf function has the same issue.
> 

Hmm, seems so. I'll make up another patch.

-- Steve


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2015-03-04 20:29 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-04 20:12 [GIT PULL] seq_buf: Fix seq_buf_vprintf() truncation Steven Rostedt
2015-03-04 20:21 ` Joe Perches
2015-03-04 20:29   ` Steven Rostedt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).