lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <alpine.LNX.2.00.1102101901420.26035@swampdragon.chaosbits.net> Date: Thu, 10 Feb 2011 19:07:47 +0100 (CET) From: Jesper Juhl <jj@...osbits.net> To: Tadashi Abe <tabe@...sta.com> cc: x86@...nel.org, tglx@...utronix.de, hpa@...or.com, mingo@...hat.com, linux-kernel@...r.kernel.org Subject: Re: [PATCH] x86: Fix io_bitmap_ptr memory leak on copy_process() On Tue, 8 Feb 2011, Tadashi Abe wrote: > x86 copy_thread() allocates io_bitmap_ptr area in child's thread_struct > when the parent has TIF_IO_BITMAP flag via ioperm(). > And in this case, if copy_process() terminates with errors > after copy_thread() success, > this io_bitmap_ptr area is being left without kfree(). > > Signed-off-by: Tadashi Abe <tabe@...sta.com> > --- > arch/x86/kernel/process.c | 6 ++++++ > include/linux/sched.h | 5 +++++ > kernel/fork.c | 4 +++- > 3 files changed, 14 insertions(+), 1 deletions(-) > > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c > index b3feabc..290e98b 100644 > --- a/arch/x86/kernel/process.c > +++ b/arch/x86/kernel/process.c > @@ -122,6 +122,12 @@ void flush_thread(void) > clear_used_math(); > } > > +void free_thread_struct(struct task_struct *p) > +{ > + if (p->thread.io_bitmap_ptr) > + kfree(p->thread.io_bitmap_ptr); kfree(NULL) is perfectly legal, so the 'if' is not needed. Why is this a new function? Why not simply call kfree() when needed. > +} > + > static void hard_disable_TSC(void) > { > write_cr4(read_cr4() | X86_CR4_TSD); > diff --git a/include/linux/sched.h b/include/linux/sched.h > index 23e9c27..2345391 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -2159,6 +2159,11 @@ extern int copy_thread(unsigned long, unsigned long, unsigned long, > struct task_struct *, struct pt_regs *); > extern void flush_thread(void); > extern void exit_thread(void); > +#ifdef CONFIG_X86 > +extern void free_thread_struct(struct task_struct *p); > +#else > +#define free_thread_struct(p) do { } while (0) > +#endif > This should just go away IMHO. > extern void exit_files(struct task_struct *); > extern void __cleanup_sighand(struct sighand_struct *); > diff --git a/kernel/fork.c b/kernel/fork.c > index 25e4291..ffcabf3 100644 > --- a/kernel/fork.c > +++ b/kernel/fork.c > @@ -1179,7 +1179,7 @@ static struct task_struct *copy_process(unsigned long clone_flags, > retval = -ENOMEM; > pid = alloc_pid(p->nsproxy->pid_ns); > if (!pid) > - goto bad_fork_cleanup_io; > + goto bad_fork_free_thread; > > if (clone_flags & CLONE_NEWPID) { > retval = pid_ns_prepare_proc(p->nsproxy->pid_ns); > @@ -1315,6 +1315,8 @@ static struct task_struct *copy_process(unsigned long clone_flags, > bad_fork_free_pid: > if (pid != &init_struct_pid) > free_pid(pid); > +bad_fork_free_thread: > + free_thread_struct(p); kfree(p); and be done with it.. > bad_fork_cleanup_io: > if (p->io_context) > exit_io_context(p); > Perhaps I'm overlooking something, in which case I'd be happy to be corrected, but I really can't see why this can't just be as simple as I outlined above. -- Jesper Juhl <jj@...osbits.net> http://www.chaosbits.net/ Plain text mails only, please. Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists