* [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 related [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, other threads:[~2018-01-04 9:54 UTC | newest]
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
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.