LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] Fix read buffer overflow in delta-ipc
@ 2017-12-22  0:12 Andi Kleen
  2018-01-03  9:40 ` Hugues FRUCHET
  0 siblings, 1 reply; 4+ messages in thread
From: Andi Kleen @ 2017-12-22  0:12 UTC (permalink / raw)
  To: linux-kernel; +Cc: akpm, Andi Kleen, hugues.fruchet, mchehab

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

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

* Re: [PATCH] Fix read buffer overflow in delta-ipc
  2017-12-22  0:12 [PATCH] Fix read buffer overflow in delta-ipc Andi Kleen
@ 2018-01-03  9:40 ` Hugues FRUCHET
  2018-01-04  0:19   ` Andi Kleen
  0 siblings, 1 reply; 4+ messages in thread
From: Hugues FRUCHET @ 2018-01-03  9:40 UTC (permalink / raw)
  To: Andi Kleen, linux-kernel; +Cc: akpm, Andi Kleen, mchehab

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);
> 

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

* Re: [PATCH] Fix read buffer overflow in delta-ipc
  2018-01-03  9:40 ` Hugues FRUCHET
@ 2018-01-04  0:19   ` Andi Kleen
  2018-01-04  9:53     ` Hugues FRUCHET
  0 siblings, 1 reply; 4+ messages in thread
From: Andi Kleen @ 2018-01-04  0:19 UTC (permalink / raw)
  To: Hugues FRUCHET; +Cc: Andi Kleen, linux-kernel, akpm, Andi Kleen, mchehab

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

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

* Re: [PATCH] Fix read buffer overflow in delta-ipc
  2018-01-04  0:19   ` Andi Kleen
@ 2018-01-04  9:53     ` Hugues FRUCHET
  0 siblings, 0 replies; 4+ messages in thread
From: Hugues FRUCHET @ 2018-01-04  9:53 UTC (permalink / raw)
  To: Andi Kleen; +Cc: linux-kernel, akpm, Andi Kleen, mchehab

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
> 

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

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-22  0:12 [PATCH] Fix read buffer overflow in delta-ipc Andi Kleen
2018-01-03  9:40 ` Hugues FRUCHET
2018-01-04  0:19   ` Andi Kleen
2018-01-04  9:53     ` Hugues FRUCHET

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox