Fix read buffer overflow in delta-ipc
PATCH 1/1 Fix read buffer overflow in delta-ipc
From: Andi Kleen <[email protected]>
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();
--- drivers/media/platform/sti/delta/delta-ipc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/platform/sti/delta/delta-ipc.c +2 -2
@@ -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);
Signature · 1 line
2.15.0
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.
Quoted 44 lines · Andi Kleen
From: Andi Kleen <[email protected]>
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’ atlinux/drivers/media/platform/sti/delta/delta-ipc.c:178:0,
inlined from ‘delta_mjpeg_ipc_open’ atlinux/drivers/media/platform/sti/delta/delta-mjpeg-dec.c:227:0,
inlined from ‘delta_mjpeg_decode’ atlinux/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();--- drivers/media/platform/sti/delta/delta-ipc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/media/platform/sti/delta/delta-ipc.c +2
-2@@ -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);
drivers/media/platform/sti/delta/delta-ipc.c int delta_ipc_open(struct delta_ctx *pctx, const char *name,
Quoted 7 lines · 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:drivers/media/platform/sti/delta/delta-ipc.c +1
-2-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
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.
Quoted 17 lines · Andi Kleen
drivers/media/platform/sti/delta/delta-ipc.c int delta_ipc_open(struct delta_ctx *pctx, const char *name,
Quoted 7 lines · 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:drivers/media/platform/sti/delta/delta-ipc.c +1
-2-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