[PATCH] vmcore-dmesg stack smashing happend in extreme case

[PATCH] vmcore-dmesg stack smashing happend in extreme case

[arthur]

Description
in dump_dmesg_structured() the out_buf size is 4096, and if the
length is less than 4080( 4096-16 ) it won't really write out.
Normally, after writing one or four chars to the out_buf, it will
check the length of out_buf. But in extreme cases, 19 chars was
written to the out_buf before checking the length. This may cause
the stack corruption. If the length was 4079 (won't realy write out),
and then write 19 chars to it. the out_buf will overflow.

Solution
Change 16 to 32 and always check the lenth of out_buf before move to
next record, thus can make sure that at the beginning of a message
we always have 32 bytes of buffer.

Signed-off-by: arthur <zzou@redhat.com>
---
 vmcore-dmesg/vmcore-dmesg.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/vmcore-dmesg/vmcore-dmesg.c b/vmcore-dmesg/vmcore-dmesg.c
index 0345660..0d323c1 100644
--- a/vmcore-dmesg/vmcore-dmesg.c
+++ b/vmcore-dmesg/vmcore-dmesg.c
@@ -674,7 +674,7 @@ static void dump_dmesg_structured(int fd)
 			else
 				out_buf[len++] = c;
 
-			if (len >= OUT_BUF_SIZE - 16) {
+			if (len >= OUT_BUF_SIZE - 32) {
 				write_to_stdout(out_buf, len);
 				len = 0;
 			}
@@ -682,6 +682,11 @@ static void dump_dmesg_structured(int fd)
 
 		out_buf[len++] = '\n';
 
+		if (len >= OUT_BUF_SIZE - 32) {
+			write_to_stdout(out_buf, len);
+			len = 0;
+		}
+
 		/* Move to next record */
 		current_idx = log_next(buf, current_idx);
 	}
-- 
1.8.4.2


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] vmcore-dmesg stack smashing happend in extreme case

[Vivek Goyal]

On Mon, Mar 10, 2014 at 05:25:11PM +0800, arthur wrote:
> Description
> in dump_dmesg_structured() the out_buf size is 4096, and if the
> length is less than 4080( 4096-16 ) it won't really write out.
> Normally, after writing one or four chars to the out_buf, it will
> check the length of out_buf. But in extreme cases, 19 chars was
> written to the out_buf before checking the length. This may cause
> the stack corruption. If the length was 4079 (won't realy write out),
> and then write 19 chars to it. the out_buf will overflow.
> 
> Solution
> Change 16 to 32 and always check the lenth of out_buf before move to
> next record, thus can make sure that at the beginning of a message
> we always have 32 bytes of buffer.
> 
> Signed-off-by: arthur <zzou@redhat.com>
> ---
>  vmcore-dmesg/vmcore-dmesg.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/vmcore-dmesg/vmcore-dmesg.c b/vmcore-dmesg/vmcore-dmesg.c
> index 0345660..0d323c1 100644
> --- a/vmcore-dmesg/vmcore-dmesg.c
> +++ b/vmcore-dmesg/vmcore-dmesg.c
> @@ -674,7 +674,7 @@ static void dump_dmesg_structured(int fd)
>  			else
>  				out_buf[len++] = c;
>  
> -			if (len >= OUT_BUF_SIZE - 16) {
> +			if (len >= OUT_BUF_SIZE - 32) {

How do you know 32 is enough and will not overflow?

>  				write_to_stdout(out_buf, len);
>  				len = 0;
>  			}
> @@ -682,6 +682,11 @@ static void dump_dmesg_structured(int fd)
>  
>  		out_buf[len++] = '\n';
>  
> +		if (len >= OUT_BUF_SIZE - 32) {
> +			write_to_stdout(out_buf, len);
> +			len = 0;
> +		}

Why do we need this? After last check only one extra character "\n"
has been written?

I think that this overflow is coming from following sprintf().

                len += sprintf(out_buf + len, "[%5llu.%06llu] ",
                        (long long unsigned int)imaxdiv_sec.quot,
                        (long long unsigned int)imaxdiv_usec.quot);

So we are printing 2 long long unsigned int and 3 more chars "[] ". I 

Max long long unsigned int is (2^64 -1) = 18446744073709551615

Above takes 20 bytes. So total max length can be 20 + 20 + 3 = 43.

I think just leave 64bytes of buffer before we enter the next message
and that should be fine.

And we don't have to do this twice. Just do it once inside innermost
for loop.

Thanks
Vivek

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] vmcore-dmesg stack smashing happend in extreme case

[Zhi Zou]

----- Original Message -----
> On Mon, Mar 10, 2014 at 05:25:11PM +0800, arthur wrote:
> > Description
> > in dump_dmesg_structured() the out_buf size is 4096, and if the
> > length is less than 4080( 4096-16 ) it won't really write out.
> > Normally, after writing one or four chars to the out_buf, it will
> > check the length of out_buf. But in extreme cases, 19 chars was
> > written to the out_buf before checking the length. This may cause
> > the stack corruption. If the length was 4079 (won't realy write out),
> > and then write 19 chars to it. the out_buf will overflow.
> > 
> > Solution
> > Change 16 to 32 and always check the lenth of out_buf before move to
> > next record, thus can make sure that at the beginning of a message
> > we always have 32 bytes of buffer.
> > 
> > Signed-off-by: arthur <zzou@redhat.com>
> > ---
> >  vmcore-dmesg/vmcore-dmesg.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/vmcore-dmesg/vmcore-dmesg.c b/vmcore-dmesg/vmcore-dmesg.c
> > index 0345660..0d323c1 100644
> > --- a/vmcore-dmesg/vmcore-dmesg.c
> > +++ b/vmcore-dmesg/vmcore-dmesg.c
> > @@ -674,7 +674,7 @@ static void dump_dmesg_structured(int fd)
> >  			else
> >  				out_buf[len++] = c;
> >  
> > -			if (len >= OUT_BUF_SIZE - 16) {
> > +			if (len >= OUT_BUF_SIZE - 32) {
> 
> How do you know 32 is enough and will not overflow?
> 
> >  				write_to_stdout(out_buf, len);
> >  				len = 0;
> >  			}
> > @@ -682,6 +682,11 @@ static void dump_dmesg_structured(int fd)
> >  
> >  		out_buf[len++] = '\n';
> >  
> > +		if (len >= OUT_BUF_SIZE - 32) {
> > +			write_to_stdout(out_buf, len);
> > +			len = 0;
> > +		}
> 
> Why do we need this? After last check only one extra character "\n"
> has been written?
> 
> I think that this overflow is coming from following sprintf().
> 
>                 len += sprintf(out_buf + len, "[%5llu.%06llu] ",
>                         (long long unsigned int)imaxdiv_sec.quot,
>                         (long long unsigned int)imaxdiv_usec.quot);
> 
> So we are printing 2 long long unsigned int and 3 more chars "[] ". I
> 
> Max long long unsigned int is (2^64 -1) = 18446744073709551615
> 
> Above takes 20 bytes. So total max length can be 20 + 20 + 3 = 43.
> 
> I think just leave 64bytes of buffer before we enter the next message
> and that should be fine.

Hi Vivek
You are right. we need leave 64bytes instead of 32bytes.

> 
> And we don't have to do this twice. Just do it once inside innermost
> for loop.

At the beginning, I think it is unnecessary to do this twice too. But
if the text_len ==0  then the for loop won't be executed and check won't
be executed either. May be this situation never come up, considering it's
cost, I can accept it because most of time we just do the check instead of
write. If it is sure that text_len == 0 never come up, then we should remove
it.

Thanks
zzou

> 
> Thanks
> Vivek
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] vmcore-dmesg stack smashing happend in extreme case

[Vivek Goyal]

On Mon, Mar 10, 2014 at 09:42:18PM -0400, Zhi Zou wrote:

[..]
> > 
> > And we don't have to do this twice. Just do it once inside innermost
> > for loop.
> 
> At the beginning, I think it is unnecessary to do this twice too. But
> if the text_len ==0  then the for loop won't be executed and check won't
> be executed either. May be this situation never come up, considering it's
> cost, I can accept it because most of time we just do the check instead of
> write. If it is sure that text_len == 0 never come up, then we should remove
> it.

When do we get text_len=0? If text_len=0, then we are just printing time
stamp? Does it really happen?

I think do a quick test and if we are not encountering such cases, don't
worry about it right now. Making logic too complicated does not help.

Thanks
Vivek

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec