[PATCH] Fix read buffer overflow in delta-ipc

[PATCH] Fix read buffer overflow in delta-ipc

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

The single caller passes a string to delta_ipc_open, which copies with a
fixed size larger than the string. So it copies some random data after
the original string the ro segment.

If the string was at the end of a page it may fault.

Just copy the string with a normal strcpy after clearing the field.

Found by a LTO build (which errors out)
because the compiler inlines the functions and can resolve
the string sizes and triggers the compile time checks in memcpy.

In function ‘memcpy’,
    inlined from ‘delta_ipc_open.constprop’ at linux/drivers/media/platform/sti/delta/delta-ipc.c:178:0,
    inlined from ‘delta_mjpeg_ipc_open’ at linux/drivers/media/platform/sti/delta/delta-mjpeg-dec.c:227:0,
    inlined from ‘delta_mjpeg_decode’ at linux/drivers/media/platform/sti/delta/delta-mjpeg-dec.c:403:0:
/home/andi/lsrc/linux/include/linux/string.h:337:0: error: call to ‘__read_overflow2’ declared with attribute error: detected read beyond size of object passed as 2nd parameter
    __read_overflow2();

Cc: hugues.fruchet@st.com
Cc: mchehab@s-opensource.com
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 drivers/media/platform/sti/delta/delta-ipc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/sti/delta/delta-ipc.c b/drivers/media/platform/sti/delta/delta-ipc.c
index 41e4a4c259b3..b6c256e3ceb6 100644
--- a/drivers/media/platform/sti/delta/delta-ipc.c
+++ b/drivers/media/platform/sti/delta/delta-ipc.c
@@ -175,8 +175,8 @@ int delta_ipc_open(struct delta_ctx *pctx, const char *name,
 	msg.ipc_buf_size = ipc_buf_size;
 	msg.ipc_buf_paddr = ctx->ipc_buf->paddr;
 
-	memcpy(msg.name, name, sizeof(msg.name));
-	msg.name[sizeof(msg.name) - 1] = 0;
+	memset(msg.name, 0, sizeof(msg.name));
+	strcpy(msg.name, name);
 
 	msg.param_size = param->size;
 	memcpy(ctx->ipc_buf->vaddr, param->data, msg.param_size);
-- 
2.15.0

Re: [PATCH] Fix read buffer overflow in delta-ipc

[Hugues FRUCHET]

Hi Andi,
Thanks for the patch but I would suggest to use strlcpy instead, this 
will guard msg.name overwriting and add the NULL termination in case
of truncation:
-	memcpy(msg.name, name, sizeof(msg.name));
-	msg.name[sizeof(msg.name) - 1] = 0;
+	strlcpy(msg.name, name, sizeof(msg.name));

Best regards,
Hugues.

On 12/22/2017 01:12 AM, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> The single caller passes a string to delta_ipc_open, which copies with a
> fixed size larger than the string. So it copies some random data after
> the original string the ro segment.
> 
> If the string was at the end of a page it may fault.
> 
> Just copy the string with a normal strcpy after clearing the field.
> 
> Found by a LTO build (which errors out)
> because the compiler inlines the functions and can resolve
> the string sizes and triggers the compile time checks in memcpy.
> 
> In function ‘memcpy’,
>      inlined from ‘delta_ipc_open.constprop’ at linux/drivers/media/platform/sti/delta/delta-ipc.c:178:0,
>      inlined from ‘delta_mjpeg_ipc_open’ at linux/drivers/media/platform/sti/delta/delta-mjpeg-dec.c:227:0,
>      inlined from ‘delta_mjpeg_decode’ at linux/drivers/media/platform/sti/delta/delta-mjpeg-dec.c:403:0:
> /home/andi/lsrc/linux/include/linux/string.h:337:0: error: call to ‘__read_overflow2’ declared with attribute error: detected read beyond size of object passed as 2nd parameter
>      __read_overflow2();
> 
> Cc: hugues.fruchet@st.com
> Cc: mchehab@s-opensource.com
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>   drivers/media/platform/sti/delta/delta-ipc.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/sti/delta/delta-ipc.c b/drivers/media/platform/sti/delta/delta-ipc.c
> index 41e4a4c259b3..b6c256e3ceb6 100644
> --- a/drivers/media/platform/sti/delta/delta-ipc.c
> +++ b/drivers/media/platform/sti/delta/delta-ipc.c
> @@ -175,8 +175,8 @@ int delta_ipc_open(struct delta_ctx *pctx, const char *name,
>   	msg.ipc_buf_size = ipc_buf_size;
>   	msg.ipc_buf_paddr = ctx->ipc_buf->paddr;
>   
> -	memcpy(msg.name, name, sizeof(msg.name));
> -	msg.name[sizeof(msg.name) - 1] = 0;
> +	memset(msg.name, 0, sizeof(msg.name));
> +	strcpy(msg.name, name);
>   
>   	msg.param_size = param->size;
>   	memcpy(ctx->ipc_buf->vaddr, param->data, msg.param_size);
> 

Re: [PATCH] Fix read buffer overflow in delta-ipc

[Andi Kleen]

On Wed, Jan 03, 2018 at 09:40:04AM +0000, Hugues FRUCHET wrote:
> Hi Andi,
> Thanks for the patch but I would suggest to use strlcpy instead, this 
> will guard msg.name overwriting and add the NULL termination in case
> of truncation:
> -	memcpy(msg.name, name, sizeof(msg.name));
> -	msg.name[sizeof(msg.name) - 1] = 0;
> +	strlcpy(msg.name, name, sizeof(msg.name));

I'm not an expert on your setup, but it seems strlcpy would leak some
uninitialized stack data over your ipc mechanism. strclpy doesn't pad the
data. If the IPC is a security boundary that would be a security bug.

So I think the original patch is better than strlcpy.

-Andi

Re: [PATCH] Fix read buffer overflow in delta-ipc

[Hugues FRUCHET]

Hi Andi,

Anyway we cannot keep strcpy, if name is not NULL terminated case, 
msg.name is overflowed.
Trying to find some safe design pattern about that, I've found strscpy:
https://lwn.net/Articles/643376/
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=30c44659f4a3e7e1f9f47e895591b4b40bf62671

which clearly indicates error in case of overflow, so code becomes:
-	memcpy(msg.name, name, sizeof(msg.name));
-	msg.name[sizeof(msg.name) - 1] = 0;
+	if (strscpy(msg.name, name, sizeof(msg.name)) <= 0)
+         goto err;

What do you think of this proposal ?

Best regards,
Hugues.

On 01/04/2018 01:19 AM, Andi Kleen wrote:
> On Wed, Jan 03, 2018 at 09:40:04AM +0000, Hugues FRUCHET wrote:
>> Hi Andi,
>> Thanks for the patch but I would suggest to use strlcpy instead, this
>> will guard msg.name overwriting and add the NULL termination in case
>> of truncation:
>> -	memcpy(msg.name, name, sizeof(msg.name));
>> -	msg.name[sizeof(msg.name) - 1] = 0;
>> +	strlcpy(msg.name, name, sizeof(msg.name));
> 
> I'm not an expert on your setup, but it seems strlcpy would leak some
> uninitialized stack data over your ipc mechanism. strclpy doesn't pad the
> data. If the IPC is a security boundary that would be a security bug.
> 
> So I think the original patch is better than strlcpy.
> 
> -Andi
>