* [RFC PATCH 1/2] binfmt_elf: Fix corner case kfree of uninitialized data
@ 2012-09-19 14:44 Alan Cox
2012-09-19 14:45 ` [RFC PATCH 2/2] binfmt_elf: Uninitialized variable Alan Cox
2012-09-26 23:21 ` [RFC PATCH 1/2] binfmt_elf: Fix corner case kfree of uninitialized data Andrew Morton
0 siblings, 2 replies; 4+ messages in thread
From: Alan Cox @ 2012-09-19 14:44 UTC (permalink / raw)
To: linux-kernel
Could do with double checking...
From: Alan Cox <alan@linux.intel.com>
If elf_core_dump is called and fill_note_info fails in the kmalloc then
it returns 0 but has not yet initialised all the needed fields. As a result
we do a kfree(randomness) after correctly skipping the thread data.
Signed-off-by: Alan Cox <alan@linux.intel.com>
---
fs/binfmt_elf.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 1b4efbc..bf6d82b 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1492,8 +1492,10 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
info->thread = NULL;
psinfo = kmalloc(sizeof(*psinfo), GFP_KERNEL);
- if (psinfo == NULL)
+ if (psinfo == NULL) {
+ info->psinfo.data = NULL; /* So we don't free this wrongly */
return 0;
+ }
fill_note(&info->psinfo, "CORE", NT_PRPSINFO, sizeof(*psinfo), psinfo);
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [RFC PATCH 2/2] binfmt_elf: Uninitialized variable
2012-09-19 14:44 [RFC PATCH 1/2] binfmt_elf: Fix corner case kfree of uninitialized data Alan Cox
@ 2012-09-19 14:45 ` Alan Cox
2012-09-26 23:25 ` Andrew Morton
2012-09-26 23:21 ` [RFC PATCH 1/2] binfmt_elf: Fix corner case kfree of uninitialized data Andrew Morton
1 sibling, 1 reply; 4+ messages in thread
From: Alan Cox @ 2012-09-19 14:45 UTC (permalink / raw)
To: linux-kernel
From: Alan Cox <alan@linux.intel.com>
load_elf_interp has interp_map_addr carefully described as
"uninitialized_var" and marked so as to avoid a warning. However
if you trace the code it is passed into load_elf_interp and then
this value is checked against NULL.
As this return value isn't used this is actually safe but it freaks
various analysis tools that see un-initialized memory addresses
being read before their value is ever defined.
Set it to NULL as a matter of programming good taste if nothing else
Signed-off-by: Alan Cox <alan@linux.intel.com>
---
fs/binfmt_elf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index bf6d82b..5fb4801 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -880,7 +880,7 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
}
if (elf_interpreter) {
- unsigned long uninitialized_var(interp_map_addr);
+ unsigned long interp_map_addr = 0;
elf_entry = load_elf_interp(&loc->interp_elf_ex,
interpreter,
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCH 1/2] binfmt_elf: Fix corner case kfree of uninitialized data
2012-09-19 14:44 [RFC PATCH 1/2] binfmt_elf: Fix corner case kfree of uninitialized data Alan Cox
2012-09-19 14:45 ` [RFC PATCH 2/2] binfmt_elf: Uninitialized variable Alan Cox
@ 2012-09-26 23:21 ` Andrew Morton
1 sibling, 0 replies; 4+ messages in thread
From: Andrew Morton @ 2012-09-26 23:21 UTC (permalink / raw)
To: Alan Cox; +Cc: linux-kernel
On Wed, 19 Sep 2012 15:44:35 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> Could do with double checking...
>
> From: Alan Cox <alan@linux.intel.com>
>
> If elf_core_dump is called and fill_note_info fails in the kmalloc then
> it returns 0 but has not yet initialised all the needed fields. As a result
> we do a kfree(randomness) after correctly skipping the thread data.
>
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> ---
>
> fs/binfmt_elf.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 1b4efbc..bf6d82b 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1492,8 +1492,10 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
> info->thread = NULL;
>
> psinfo = kmalloc(sizeof(*psinfo), GFP_KERNEL);
> - if (psinfo == NULL)
> + if (psinfo == NULL) {
> + info->psinfo.data = NULL; /* So we don't free this wrongly */
> return 0;
> + }
>
> fill_note(&info->psinfo, "CORE", NT_PRPSINFO, sizeof(*psinfo), psinfo);
afaict it's NotABug, because fill_note_info() does
info->thread = NULL;
psinfo = kmalloc(sizeof(*psinfo), GFP_KERNEL);
if (psinfo == NULL) {
and free_note_info() does
struct elf_thread_core_info *threads = info->thread;
while (threads) {
so free_note_info() won't enter the freeing loop at all.
Which is just as well, because info->thread_notes is uninitialised at
this time.
It all looks rather fragile - I'm wondering if it would be sanest to
memset `info' right at the outset in elf_core_dump(), then weed out all
the now-unneeded zeroizings in fill_note_info().
Also, how irritating is it that fill_note_info() has a local var
`psinfo' which has a different type from info->psinfo. That had me
running around for a while...
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH 2/2] binfmt_elf: Uninitialized variable
2012-09-19 14:45 ` [RFC PATCH 2/2] binfmt_elf: Uninitialized variable Alan Cox
@ 2012-09-26 23:25 ` Andrew Morton
0 siblings, 0 replies; 4+ messages in thread
From: Andrew Morton @ 2012-09-26 23:25 UTC (permalink / raw)
To: Alan Cox; +Cc: linux-kernel
On Wed, 19 Sep 2012 15:45:01 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> load_elf_interp has interp_map_addr carefully described as
> "uninitialized_var" and marked so as to avoid a warning. However
> if you trace the code it is passed into load_elf_interp and then
> this value is checked against NULL.
>
> As this return value isn't used this is actually safe but it freaks
> various analysis tools that see un-initialized memory addresses
> being read before their value is ever defined.
>
> Set it to NULL as a matter of programming good taste if nothing else
>
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> ---
>
> fs/binfmt_elf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index bf6d82b..5fb4801 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -880,7 +880,7 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
> }
>
> if (elf_interpreter) {
> - unsigned long uninitialized_var(interp_map_addr);
> + unsigned long interp_map_addr = 0;
>
> elf_entry = load_elf_interp(&loc->interp_elf_ex,
> interpreter,
That looks right to me.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-09-26 23:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-19 14:44 [RFC PATCH 1/2] binfmt_elf: Fix corner case kfree of uninitialized data Alan Cox
2012-09-19 14:45 ` [RFC PATCH 2/2] binfmt_elf: Uninitialized variable Alan Cox
2012-09-26 23:25 ` Andrew Morton
2012-09-26 23:21 ` [RFC PATCH 1/2] binfmt_elf: Fix corner case kfree of uninitialized data Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).