[PATCH] efi: fix out-of-bounds null overwrite vulnerability

[PATCH] efi: fix out-of-bounds null overwrite vulnerability

[Insu Yun]

snprintf's return value is not bound by size value.
(https://www.kernel.org/doc/htmldocs/kernel-api/API-snprintf.html)
if printed value is larger than buffer size, it can overwrite 
null byte in out-of-bounds buffer.

Signed-off-by: Insu Yun <wuninsu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efi/cper.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index d425374..77aa75f 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -267,7 +267,6 @@ static int cper_dimm_err_location(struct cper_mem_err_compact *mem, char *msg)
 			     "DIMM location: not present. DMI handle: 0x%.4x ",
 			     mem->mem_dev_handle);
 
-	msg[n] = '\0';
 	return n;
 }
 
-- 
1.9.1

[PATCH] efi: fix out-of-bounds null overwrite vulnerability

[Insu Yun]

snprintf's return value is not bound by size value.
(https://www.kernel.org/doc/htmldocs/kernel-api/API-snprintf.html)
if printed value is larger than buffer size, it can overwrite 
null byte in out-of-bounds buffer.

Signed-off-by: Insu Yun <wuninsu@gmail.com>
---
 drivers/firmware/efi/cper.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index d425374..77aa75f 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -267,7 +267,6 @@ static int cper_dimm_err_location(struct cper_mem_err_compact *mem, char *msg)
 			     "DIMM location: not present. DMI handle: 0x%.4x ",
 			     mem->mem_dev_handle);
 
-	msg[n] = '\0';
 	return n;
 }
 
-- 
1.9.1

Re: [PATCH] efi: fix out-of-bounds null overwrite vulnerability

[Matt Fleming]

On Thu, 07 Jan, at 02:05:30PM, Insu Yun wrote:
> snprintf's return value is not bound by size value.
> (https://www.kernel.org/doc/htmldocs/kernel-api/API-snprintf.html)
> if printed value is larger than buffer size, it can overwrite 
> null byte in out-of-bounds buffer.
 
But this function doesn't use snprintf(), it uses scnprintf() which
returns the number of characters written into buf and, because
scnprintf() largely follows vnsprintf(), it will never write more than
'size' bytes into the buffer.

> Signed-off-by: Insu Yun <wuninsu-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/firmware/efi/cper.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index d425374..77aa75f 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -267,7 +267,6 @@ static int cper_dimm_err_location(struct cper_mem_err_compact *mem, char *msg)
>  			     "DIMM location: not present. DMI handle: 0x%.4x ",
>  			     mem->mem_dev_handle);
>  
> -	msg[n] = '\0';
>  	return n;
>  }
>  

Calling this a vulnerability is a little extreme. These fields come
from firmware and if you can't trust the firmware you've got bigger
issues.

I'm not even sure this is a bug. Tony?

Re: [PATCH] efi: fix out-of-bounds null overwrite vulnerability

[Matt Fleming]

On Thu, 07 Jan, at 02:05:30PM, Insu Yun wrote:
> snprintf's return value is not bound by size value.
> (https://www.kernel.org/doc/htmldocs/kernel-api/API-snprintf.html)
> if printed value is larger than buffer size, it can overwrite 
> null byte in out-of-bounds buffer.
 
But this function doesn't use snprintf(), it uses scnprintf() which
returns the number of characters written into buf and, because
scnprintf() largely follows vnsprintf(), it will never write more than
'size' bytes into the buffer.

> Signed-off-by: Insu Yun <wuninsu@gmail.com>
> ---
>  drivers/firmware/efi/cper.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index d425374..77aa75f 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -267,7 +267,6 @@ static int cper_dimm_err_location(struct cper_mem_err_compact *mem, char *msg)
>  			     "DIMM location: not present. DMI handle: 0x%.4x ",
>  			     mem->mem_dev_handle);
>  
> -	msg[n] = '\0';
>  	return n;
>  }
>  

Calling this a vulnerability is a little extreme. These fields come
from firmware and if you can't trust the firmware you've got bigger
issues.

I'm not even sure this is a bug. Tony?

RE: [PATCH] efi: fix out-of-bounds null overwrite vulnerability

[Luck, Tony]

> But this function doesn't use snprintf(), it uses scnprintf() which
> returns the number of characters written into buf and, because
> scnprintf() largely follows vnsprintf(), it will never write more than
> 'size' bytes into the buffer.

        if (bank && device)
                n = snprintf(msg, len, "DIMM location: %s %s ", bank, device);

That looks like "snprintf", not "scnprintf" to me :-)

What about using:

	msg[len] = '\0';

to guarantee NUL termination?

-Tony

RE: [PATCH] efi: fix out-of-bounds null overwrite vulnerability

[Luck, Tony]

> But this function doesn't use snprintf(), it uses scnprintf() which
> returns the number of characters written into buf and, because
> scnprintf() largely follows vnsprintf(), it will never write more than
> 'size' bytes into the buffer.

        if (bank && device)
                n = snprintf(msg, len, "DIMM location: %s %s ", bank, device);

That looks like "snprintf", not "scnprintf" to me :-)

What about using:

	msg[len] = '\0';

to guarantee NUL termination?

-Tony

Re: [PATCH] efi: fix out-of-bounds null overwrite vulnerability

[Matt Fleming]

On Fri, 08 Jan, at 04:47:17PM, Luck, Tony wrote:
> > But this function doesn't use snprintf(), it uses scnprintf() which
> > returns the number of characters written into buf and, because
> > scnprintf() largely follows vnsprintf(), it will never write more than
> > 'size' bytes into the buffer.
> 
>         if (bank && device)
>                 n = snprintf(msg, len, "DIMM location: %s %s ", bank, device);
> 
> That looks like "snprintf", not "scnprintf" to me :-)
 
Oops! Can you believe I looked at the wrong function?

> What about using:
> 
> 	msg[len] = '\0';
> 
> to guarantee NUL termination?

But that may leave garbage bytes in 'rcd_decode_str' in the case where
the string isn't as long as 'len'.

How about memset()'ing the buffer to zero and deleting the NUL
termination line?

Re: [PATCH] efi: fix out-of-bounds null overwrite vulnerability

[Matt Fleming]

On Fri, 08 Jan, at 04:47:17PM, Luck, Tony wrote:
> > But this function doesn't use snprintf(), it uses scnprintf() which
> > returns the number of characters written into buf and, because
> > scnprintf() largely follows vnsprintf(), it will never write more than
> > 'size' bytes into the buffer.
> 
>         if (bank && device)
>                 n = snprintf(msg, len, "DIMM location: %s %s ", bank, device);
> 
> That looks like "snprintf", not "scnprintf" to me :-)
 
Oops! Can you believe I looked at the wrong function?

> What about using:
> 
> 	msg[len] = '\0';
> 
> to guarantee NUL termination?

But that may leave garbage bytes in 'rcd_decode_str' in the case where
the string isn't as long as 'len'.

How about memset()'ing the buffer to zero and deleting the NUL
termination line?

RE: [PATCH] efi: fix out-of-bounds null overwrite vulnerability

[Luck, Tony]

>> What about using:
>> 
>> 	msg[len] = '\0';
>> 
>> to guarantee NUL termination?
>
> But that may leave garbage bytes in 'rcd_decode_str' in the case where
> the string isn't as long as 'len'.
>
> How about memset()'ing the buffer to zero and deleting the NUL
> termination line?

Looks like overkill to me.  We only use this function in two places:

        if (cper_dimm_err_location(cmem, rcd_decode_str))
                trace_seq_printf(p, "%s", rcd_decode_str);

        if (cper_dimm_err_location(&cmem, rcd_decode_str))
                printk("%s%s\n", pfx, rcd_decode_str);

Neither would care if there were garbage after the NUL and before the
end of the rcd_decode_str[] array.

This buffer isn't visible to user space, so we aren't leaking data by having
garbage bytes after the NUL.

-Tony

RE: [PATCH] efi: fix out-of-bounds null overwrite vulnerability

[Luck, Tony]

>> What about using:
>> 
>> 	msg[len] = '\0';
>> 
>> to guarantee NUL termination?
>
> But that may leave garbage bytes in 'rcd_decode_str' in the case where
> the string isn't as long as 'len'.
>
> How about memset()'ing the buffer to zero and deleting the NUL
> termination line?

Looks like overkill to me.  We only use this function in two places:

        if (cper_dimm_err_location(cmem, rcd_decode_str))
                trace_seq_printf(p, "%s", rcd_decode_str);

        if (cper_dimm_err_location(&cmem, rcd_decode_str))
                printk("%s%s\n", pfx, rcd_decode_str);

Neither would care if there were garbage after the NUL and before the
end of the rcd_decode_str[] array.

This buffer isn't visible to user space, so we aren't leaking data by having
garbage bytes after the NUL.

-Tony

Re: [PATCH] efi: fix out-of-bounds null overwrite vulnerability

[Matt Fleming]

On Mon, 11 Jan, at 06:16:14PM, Luck, Tony wrote:
> >> What about using:
> >> 
> >> 	msg[len] = '\0';
> >> 
> >> to guarantee NUL termination?
> >
> > But that may leave garbage bytes in 'rcd_decode_str' in the case where
> > the string isn't as long as 'len'.
> >
> > How about memset()'ing the buffer to zero and deleting the NUL
> > termination line?
> 
> Looks like overkill to me.  We only use this function in two places:
> 
>         if (cper_dimm_err_location(cmem, rcd_decode_str))
>                 trace_seq_printf(p, "%s", rcd_decode_str);
> 
>         if (cper_dimm_err_location(&cmem, rcd_decode_str))
>                 printk("%s%s\n", pfx, rcd_decode_str);
> 
> Neither would care if there were garbage after the NUL and before the
> end of the rcd_decode_str[] array.
> 
> This buffer isn't visible to user space, so we aren't leaking data by having
> garbage bytes after the NUL.

What about *before* the NUL? That was the point I was trying to make.
If the string you print into the buffer isn't 'len' bytes in size the
buffer will look like,

  "DIMM location: Foo bar"<garbage.....>\0

Doing the memset() before the call to snprintf() guarantees this won't
happen and means you don't have to try and calculate where the NUL
needs to be placed.

Re: [PATCH] efi: fix out-of-bounds null overwrite vulnerability

[Tony Luck]

On Thu, Jan 14, 2016 at 3:12 AM, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> What about *before* the NUL? That was the point I was trying to make.
> If the string you print into the buffer isn't 'len' bytes in size the
> buffer will look like,
>
>   "DIMM location: Foo bar<garbage.....>\0"

Doesn't that actually look like:

    "DIMM location: Foo bar\0<garbage.....>\0"

snprintf() will NUL terminate what it writes into the buffer
if the buffer is big enough for what you were trying to
write there.  The problem happens when what you want
to write is too big. Then you have

    "DIMM location: ReallyLongFoo AlsoBar"


at which point "msg[len] = '\0'; will zap that 'r'
at the end to make sure printk() will stop at the
end of the valid part of buffer.

-Tony

Re: [PATCH] efi: fix out-of-bounds null overwrite vulnerability

[Tony Luck]

On Thu, Jan 14, 2016 at 3:12 AM, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> What about *before* the NUL? That was the point I was trying to make.
> If the string you print into the buffer isn't 'len' bytes in size the
> buffer will look like,
>
>   "DIMM location: Foo bar<garbage.....>\0"

Doesn't that actually look like:

    "DIMM location: Foo bar\0<garbage.....>\0"

snprintf() will NUL terminate what it writes into the buffer
if the buffer is big enough for what you were trying to
write there.  The problem happens when what you want
to write is too big. Then you have

    "DIMM location: ReallyLongFoo AlsoBar"


at which point "msg[len] = '\0'; will zap that 'r'
at the end to make sure printk() will stop at the
end of the valid part of buffer.

-Tony