uprobes: preparations for arm port
Subsystems (4)
Oleg Nesterov
maintainerIngo, please pull from
git://git.kernel.org/pub/scm/linux/kernel/git/oleg/misc uprobes/core
I have also attached the cumulative diff below. As you can see, the changes are really simple and there are no functional changes.
However I think it makes sense to merge this now, this way the upcoming arm port doesn't (almost) need the changes outside of arch/arm and thus it would be simpler to route everything via arm trees.
David A. Long (1):
uprobes: Move function declarations out of arch
Oleg Nesterov (3):
uprobes: Kill module_init() and module_exit()
uprobes: Introduce arch_uprobe->ixol
uprobes: Export write_opcode() as uprobe_write_opcode()
arch/powerpc/include/asm/uprobes.h | 8 +------- arch/x86/include/asm/uprobes.h | 12 ++++-------- include/linux/uprobes.h | 9 +++++++++ kernel/events/uprobes.c | 24 ++++++++++-------------- 4 files changed, 24 insertions(+), 29 deletions(-)
arch/powerpc/include/asm/uprobes.h +1 -7
@@ -37,6 +37,7 @@ typedef ppc_opcode_t uprobe_opcode_t;struct arch_uprobe {union {u8 insn[MAX_UINSN_BYTES ]; +u8 ixol[MAX_UINSN_BYTES ];u32 ainsn; }; };
@@ -45,11 +46,4 @@ struct arch_uprobe_task { unsigned long saved_trap_nr; }; -extern int arch_uprobe_analyze_insn (struct arch_uprobe *aup,struct mm_struct *mm,unsigned long addr); -extern int arch_uprobe_pre_xol (struct arch_uprobe *aup,struct pt_regs *regs); -extern int arch_uprobe_post_xol (struct arch_uprobe *aup,struct pt_regs *regs); -extern bool arch_uprobe_xol_was_trapped (struct task_struct *tsk); -extern int arch_uprobe_exception_notify (struct notifier_block *self,unsigned long val,void *data); -extern void arch_uprobe_abort_xol (struct arch_uprobe *aup,struct pt_regs *regs); -extern unsigned long arch_uretprobe_hijack_return_addr (unsigned long trampoline_vaddr,struct pt_regs *regs);#endif /* _ASM_UPROBES_H */
arch/x86/include/asm/uprobes.h +4 -8
@@ -35,7 +35,10 @@ typedef u8 uprobe_opcode_t;struct arch_uprobe {u16 fixups; -u8 insn[MAX_UINSN_BYTES ]; +union { +u8 insn[MAX_UINSN_BYTES ]; +u8 ixol[MAX_UINSN_BYTES ]; + };#ifdef CONFIG_X86_64 unsigned long rip_rela_target_address;#endif
@@ -49,11 +52,4 @@ struct arch_uprobe_task { unsigned int saved_tf; }; -extern int arch_uprobe_analyze_insn (struct arch_uprobe *aup,struct mm_struct *mm,unsigned long addr); -extern int arch_uprobe_pre_xol (struct arch_uprobe *aup,struct pt_regs *regs); -extern int arch_uprobe_post_xol (struct arch_uprobe *aup,struct pt_regs *regs); -extern bool arch_uprobe_xol_was_trapped (struct task_struct *tsk); -extern int arch_uprobe_exception_notify (struct notifier_block *self,unsigned long val,void *data); -extern void arch_uprobe_abort_xol (struct arch_uprobe *aup,struct pt_regs *regs); -extern unsigned long arch_uretprobe_hijack_return_addr (unsigned long trampoline_vaddr,struct pt_regs *regs);#endif /* _ASM_UPROBES_H */
include/linux/uprobes.h +9 -0
@@ -30,6 +30,7 @@struct vm_area_struct ;struct mm_struct ;struct inode ; +struct notifier_block ;#ifdef CONFIG_ARCH_SUPPORTS_UPROBES # include <asm/uprobes.h>
@@ -108,6 +109,7 @@ extern int __weak set_swbp(struct arch_uprobe *aup, struct mm_struct *mm, unsignextern int __weak set_orig_insn (struct arch_uprobe *aup,struct mm_struct *mm,unsigned long vaddr);extern bool __weak is_swbp_insn (uprobe_opcode_t *insn);extern bool __weak is_trap_insn (uprobe_opcode_t *insn); +extern int uprobe_write_opcode (struct mm_struct *mm,unsigned long vaddr,uprobe_opcode_t );extern int uprobe_register (struct inode *inode,loff_t offset,struct uprobe_consumer *uc);extern int uprobe_apply (struct inode *inode,loff_t offset,struct uprobe_consumer *uc,bool );extern void uprobe_unregister (struct inode *inode,loff_t offset,struct uprobe_consumer *uc);
@@ -125,6 +127,13 @@ extern void uprobe_notify_resume(struct pt_regs *regs);extern bool uprobe_deny_signal (void );extern bool arch_uprobe_skip_sstep (struct arch_uprobe *aup,struct pt_regs *regs);extern void uprobe_clear_state (struct mm_struct *mm); +extern int arch_uprobe_analyze_insn (struct arch_uprobe *aup,struct mm_struct *mm,unsigned long addr); +extern int arch_uprobe_pre_xol (struct arch_uprobe *aup,struct pt_regs *regs); +extern int arch_uprobe_post_xol (struct arch_uprobe *aup,struct pt_regs *regs); +extern bool arch_uprobe_xol_was_trapped (struct task_struct *tsk); +extern int arch_uprobe_exception_notify (struct notifier_block *self,unsigned long val,void *data); +extern void arch_uprobe_abort_xol (struct arch_uprobe *aup,struct pt_regs *regs); +extern unsigned long arch_uretprobe_hijack_return_addr (unsigned long trampoline_vaddr,struct pt_regs *regs);#else /* !CONFIG_UPROBES */ struct uprobes_state { };
kernel/events/uprobes.c +10 -14
@@ -245,12 +245,12 @@ static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t* the architecture. If an arch has variable length instruction and the * breakpoint instruction is not of the smallest length instruction * supported by that architecture then we need to modify is_trap_at_addr and -* write_opcode accordingly. This would never be a problem for archs that -* have fixed length instructions. +* uprobe_write_opcode accordingly. This would never be a problem for archs +* that have fixed length instructions. */ /* -* write_opcode - write the opcode at a given virtual address. +* uprobe_write_opcode - write the opcode at a given virtual address. * @mm: the probed process address space. * @vaddr: the virtual address to store the opcode. * @opcode: opcode to be written at @vaddr.
@@ -261,7 +261,7 @@ static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t* For mm @mm, write the opcode at @vaddr. * Return 0 (success) or a negative errno. */ -static int write_opcode (struct mm_struct *mm,unsigned long vaddr, +int uprobe_write_opcode (struct mm_struct *mm,unsigned long vaddr,uprobe_opcode_t opcode) {struct page *old_page, *new_page;
@@ -315,7 +315,7 @@ put_old:*/ int __weak set_swbp (struct arch_uprobe *auprobe,struct mm_struct *mm,unsigned long vaddr) { -return write_opcode (mm, vaddr,UPROBE_SWBP_INSN ); +return uprobe_write_opcode (mm, vaddr,UPROBE_SWBP_INSN ); }/**
@@ -330,7 +330,7 @@ int __weak set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, unsignedint __weak set_orig_insn (struct arch_uprobe *auprobe,struct mm_struct *mm,unsigned long vaddr) { -return write_opcode (mm, vaddr, *(uprobe_opcode_t *)auprobe->insn); +return uprobe_write_opcode (mm, vaddr, *(uprobe_opcode_t *)auprobe->insn); }static int match_uprobe (struct uprobe *l,struct uprobe *r)
@@ -577,7 +577,7 @@ static int prepare_uprobe(struct uprobe *uprobe, struct file *file,if (ret) goto out; -/* write_opcode() assumes we don't cross page boundary */ +/* uprobe_write_opcode() assumes we don't cross page boundary */ BUG_ON ((uprobe->offset & ~PAGE_MASK ) +UPROBE_SWBP_INSN_SIZE >PAGE_SIZE );
@@ -1264,7 +1264,8 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe)return 0 ;/* Initialize the slot */ -copy_to_page (area->page, xol_vaddr, uprobe->arch.insn,MAX_UINSN_BYTES ); +copy_to_page (area->page, xol_vaddr, + uprobe->arch.ixol,sizeof (uprobe->arch.ixol));/* * We probably need flush_icache_user_range() but it needs vma. * This should work on supported architectures too.
@@ -1941,9 +1942,4 @@ static int __init init_uprobes(void)return register_die_notifier (&uprobe_exception_nb); } -module_init (init_uprobes); - -static void __exit exit_uprobes(void) -{ -} -module_exit (exit_uprobes); +__initcall (init_uprobes);
Srikar Dronamraju
Acked-byQuoted 14 lines · Oleg Nesterov
Ingo, please pull from
git://git.kernel.org/pub/scm/linux/kernel/git/oleg/miscuprobes/coreI have also attached the cumulative diff below. As you can see, the changes are really simple and there are no functional changes.
However I think it makes sense to merge this now, this way the upcoming arm port doesn't (almost) need the changes outside of
arch/armand thus it would be simpler to route everything via arm trees.
Quoted 8 lines
David A. Long (1):
uprobes: Move function declarations out of archOleg Nesterov (3):
uprobes: Killmodule_init()andmodule_exit()
uprobes: Introducearch_uprobe->ixol
uprobes: Exportwrite_opcode()asuprobe_write_opcode()
for this series.
Signature · 2 lines
Thanks and Regards Srikar Dronamraju
Quoted 22 lines · Oleg Nesterov
Ingo, please pull from
git://git.kernel.org/pub/scm/linux/kernel/git/oleg/miscuprobes/coreI have also attached the cumulative diff below. As you can see, the changes are really simple and there are no functional changes.
However I think it makes sense to merge this now, this way the upcoming arm port doesn't (almost) need the changes outside of
arch/armand thus it would be simpler to route everything via arm trees.David A. Long (1):
uprobes: Move function declarations out of archOleg Nesterov (3):
uprobes: Killmodule_init()andmodule_exit()
uprobes: Introducearch_uprobe->ixol
uprobes: Exportwrite_opcode()asuprobe_write_opcode()
Quoted 5 lines
arch/powerpc/include/asm/uprobes.h | 8 +------- arch/x86/include/asm/uprobes.h | 12 ++++-------- include/linux/uprobes.h | 9 +++++++++ kernel/events/uprobes.c | 24 ++++++++++-------------- 4 files changed, 24 insertions(+), 29 deletions(-)
Pulled into tip:perf/core, thanks a lot Oleg!
Ingo
Quoted 15 lines · Oleg Nesterov
arch/powerpc/include/asm/uprobes.h +1
-0@@ -37,6 +37,7 @@ typedef ppc_opcode_t uprobe_opcode_t;struct arch_uprobe {union {u8 insn[MAX_UINSN_BYTES ]; +u8 ixol[MAX_UINSN_BYTES ];u32 ainsn; }; };arch/x86/include/asm/uprobes.h +0
-0@@ -35,7 +35,10 @@ typedef u8 uprobe_opcode_t;
arch/x86/include/asm/uprobes.h typedef u8 uprobe_opcode_t; · line 44
Quoted 10 lines
arch/x86/include/asm/uprobes.h +4
-1struct arch_uprobe {u16 fixups; -u8 insn[MAX_UINSN_BYTES ]; +union { +u8 insn[MAX_UINSN_BYTES ]; +u8 ixol[MAX_UINSN_BYTES ]; + };#ifdef CONFIG_X86_64 unsigned long rip_rela_target_address;#endif
Btw., at least on the surface, the powerpc and x86 definitions seem rather similar, barring senseless variations. Would it make sense to generalize the data structure a bit more?
Also, we all hate data structures that are not self-documenting. What does 'ixol' mean and what is its role? Is it obvious to the reader of that file?
Thanks,
Ingo
Oleg Nesterov
maintainerQuoted 18 lines · Ingo Molnar
Quoted 15 lines · Oleg Nesterov
arch/powerpc/include/asm/uprobes.h +1
-0@@ -37,6 +37,7 @@ typedef ppc_opcode_t uprobe_opcode_t;struct arch_uprobe {union {u8 insn[MAX_UINSN_BYTES ]; +u8 ixol[MAX_UINSN_BYTES ];u32 ainsn; }; };arch/x86/include/asm/uprobes.h +0
-0@@ -35,7 +35,10 @@ typedef u8 uprobe_opcode_t;
Quoted 14 lines
arch/x86/include/asm/uprobes.h typedef u8 uprobe_opcode_t; · line 44
Quoted 10 lines
arch/x86/include/asm/uprobes.h +4
-1struct arch_uprobe {u16 fixups; -u8 insn[MAX_UINSN_BYTES ]; +union { +u8 insn[MAX_UINSN_BYTES ]; +u8 ixol[MAX_UINSN_BYTES ]; + };#ifdef CONFIG_X86_64 unsigned long rip_rela_target_address;#endif Btw., at least on the surface, the powerpc and x86 definitions seem rather similar, barring senseless variations. Would it make sense to generalize the data structure a bit more?
Heh. You know, I have another patch, see below. It was not tested yet, it should be splitted into 3 changes, and we need to cleanup copy_insn() first. I didn't sent it now because I wanted to merge the minimal changes which allow us to avoid the new arm arch_upobe_* hooks. And of course it needs the review.
But in short, I do not think we should try to unify/generalize insn/ixol.
For the moment, please ignore the patch which adds the new ->ixol member.
The generic code knows nothing about uprobe->arch, except: arch_uprobe must have "u8 insn[MAX_UINSN_BYTES]". And this is why powerpc also has arch_uprobe->ainsn, it defines ->insn to make the generic code happy.
Fortunately, array == &array, so we can remove this dependency and just require arch_uprobe->insn of any type, this is what the patch below tries to do.
Quoted 3 lines
Also, we all hate data structures that are not self-documenting. What does 'ixol' mean and what is its role? Is it obvious to the reader of that file?
Probably it makes sense a comment near "struct uprobe" anyway, will try to do this. But I think that with the patch below the role of insn/ixol is obvious:
arch_uprobe->insn: this is where we copy the original instruction, arch/ can do whatever it wants with this data. arch_uprobe->ixol: this is what we copy into xol slot, arch/ should initialize this data.
x86 and powerpc can use the same member, arm needs to create ixol != insn.
Note: we also have set_swbp() (which doesn't use ->insn) and
set_orig_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, unsigned long vaddr)
{
return uprobe_write_opcode(mm, vaddr, *(uprobe_opcode_t *)auprobe->insn);
}I _think_ this should be cleanuped too, but I am not sure and this will conflict with the pending arm changes. So I am going to try to do this later, after we merge arm port. And this is really minor.
Oleg.
arch/powerpc/include/asm/uprobes.h | 5 ++--- arch/powerpc/kernel/uprobes.c | 2 +- kernel/events/uprobes.c | 24 +++++++++++------------- 3 files changed, 14 insertions(+), 17 deletions(-)
arch/powerpc/include/asm/uprobes.h +2 -3
@@ -36,9 +36,8 @@ typedef ppc_opcode_t uprobe_opcode_t;struct arch_uprobe {union { -u8 insn[MAX_UINSN_BYTES ]; -u8 ixol[MAX_UINSN_BYTES ]; -u32 ainsn; +u32 insn; +u32 ixol; }; };
arch/powerpc/kernel/uprobes.c +1 -1
@@ -186,7 +186,7 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)* emulate_step() returns 1 if the insn was successfully emulated. * For all other cases, we need to single-step in hardware. */ - ret =emulate_step (regs, auprobe->ainsn); + ret =emulate_step (regs, auprobe->insn);if (ret >0 )return true;
kernel/events/uprobes.c +11 -13
@@ -329,7 +329,7 @@ int __weak set_swbp(struct arch_uprobe *auprobe, struct mm_struct *mm, unsignedint __weak set_orig_insn (struct arch_uprobe *auprobe,struct mm_struct *mm,unsigned long vaddr) { -return uprobe_write_opcode (mm, vaddr, *(uprobe_opcode_t *)auprobe->insn); +return uprobe_write_opcode (mm, vaddr, *(uprobe_opcode_t *)&auprobe->insn); }static int match_uprobe (struct uprobe *l,struct uprobe *r)
@@ -504,7 +504,7 @@ static bool consumer_del(struct uprobe *uprobe, struct uprobe_consumer *uc) }static int -__copy_insn (struct address_space *mapping,struct file *filp,char *insn, +__copy_insn (struct address_space *mapping,struct file *filp,void *insn,unsigned long nbytes,loff_t offset) {struct page *page;
@@ -527,28 +527,26 @@ __copy_insn(struct address_space *mapping, struct file *filp, char *insn,static int copy_insn (struct uprobe *uprobe,struct file *filp) { -struct address_space *mapping; -unsigned long nbytes; -int bytes; +struct address_space *mapping = uprobe->inode->i_mapping; +int bytes =sizeof (uprobe->arch.insn); +void *insn = &uprobe->arch.insn; +int nbytes; nbytes =PAGE_SIZE - (uprobe->offset & ~PAGE_MASK ); - mapping = uprobe->inode->i_mapping;/* Instruction at end of binary; copy only available bytes */ -if (uprobe->offset +MAX_UINSN_BYTES > uprobe->inode->i_size) +if (uprobe->offset + bytes > uprobe->inode->i_size) bytes = uprobe->inode->i_size - uprobe->offset; -else - bytes =MAX_UINSN_BYTES ;/* Instruction at the page-boundary; copy bytes in second page */ if (nbytes < bytes) { -int err =__copy_insn (mapping, filp, uprobe->arch.insn + nbytes, +int err =__copy_insn (mapping, filp, insn + nbytes, bytes - nbytes, uprobe->offset + nbytes);if (err)return err; bytes = nbytes; } -return __copy_insn (mapping, filp, uprobe->arch.insn, bytes, uprobe->offset); +return __copy_insn (mapping, filp, insn, bytes, uprobe->offset); } static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
@@ -569,7 +567,7 @@ static int prepare_uprobe(struct uprobe *uprobe, struct file *file, goto out; ret = -ENOTSUPP ; -if (is_trap_insn ((uprobe_opcode_t *)uprobe->arch.insn)) +if (is_trap_insn ((uprobe_opcode_t *)&uprobe->arch.insn)) goto out; ret =arch_uprobe_analyze_insn (&uprobe->arch, mm, vaddr);
@@ -1257,7 +1255,7 @@ static unsigned long xol_get_insn_slot(struct uprobe *uprobe)/* Initialize the slot */ copy_to_page (area->page, xol_vaddr, - uprobe->arch.ixol,sizeof (uprobe->arch.ixol)); + &uprobe->arch.ixol,sizeof (uprobe->arch.ixol));/* * We probably need flush_icache_user_range() but it needs vma. * This should work on supported architectures too.
Quoted 34 lines · Oleg Nesterov
Quoted 18 lines · Ingo Molnar
Quoted 15 lines · Oleg Nesterov
arch/powerpc/include/asm/uprobes.h +1
-0@@ -37,6 +37,7 @@ typedef ppc_opcode_t uprobe_opcode_t;struct arch_uprobe {union {u8 insn[MAX_UINSN_BYTES ]; +u8 ixol[MAX_UINSN_BYTES ];u32 ainsn; }; };arch/x86/include/asm/uprobes.h +0
-0@@ -35,7 +35,10 @@ typedef u8 uprobe_opcode_t;Quoted 14 lines
arch/x86/include/asm/uprobes.h typedef u8 uprobe_opcode_t; · line 44
Quoted 10 lines
arch/x86/include/asm/uprobes.h +4
-1struct arch_uprobe {u16 fixups; -u8 insn[MAX_UINSN_BYTES ]; +union { +u8 insn[MAX_UINSN_BYTES ]; +u8 ixol[MAX_UINSN_BYTES ]; + };#ifdef CONFIG_X86_64 unsigned long rip_rela_target_address;#endif Btw., at least on the surface, the powerpc and x86 definitions seem rather similar, barring senseless variations. Would it make sense to generalize the data structure a bit more?
Quoted 8 lines
Heh. You know, I have another patch, see below. It was not tested yet, it should be splitted into 3 changes, and we need to cleanup
copy_insn()first. I didn't sent it now because I wanted to merge the minimal changes which allow us to avoid the new arm arch_upobe_* hooks. And of course it needs the review.But in short, I do not think we should try to unify/generalize insn/ixol.
That's OK.
Quoted 2 lines
For the moment, please ignore the patch which adds the new ->ixol member.
I didn't actually disagree with it so I pulled it - I was just wondering about those cleanliness details.
Thanks,
Ingo
Oleg Nesterov
maintainerQuoted 5 lines · Ingo Molnar
Quoted 2 lines · Oleg Nesterov
and we need to cleanup
copy_insn()
first.
Heh. I never read it carefully, I always knew it should be cleanuped.
But when I looked at it now I realized that it is very wrong, and it is very easy to crash the kernel (fortunately only root can enable uprobes).
So we need to fix it (and cleanup), I'll try to make the patch asap.
Quoted 4 lines
Quoted 2 lines
For the moment, please ignore the patch which adds the new ->ixol member.
I didn't actually disagree with it so I pulled it
Thanks,
Quoted 2 lines
- I was just wondering
about those cleanliness details.
And you are right, this doesn't look clean. I should have mentioned in the changelog that we need to (at least try) to cleanup this all.
Oleg.
Oleg Nesterov
maintainercover uprobes: Fix the memory out of bound overwrite in copy_insn()
Quoted 6 lines · Oleg Nesterov
But when I looked at it now I realized that it is very wrong, and it is very easy to crash the kernel (fortunately only root can enable uprobes).
So we need to fix it (and cleanup), I'll try to make the patch asap.
Just truncate the binary after uprobe was enabled, then mmap() the truncated area. This crashes the kernel if nobody else mmaped this area in between.
Oleg.
Oleg Nesterov
maintainerPATCH 1/1 uprobes: Fix the memory out of bound overwrite in copy_insn()
1. copy_insn() doesn't look very nice, all calculations are confusing and it is not immediately clear why do we read the 2nd page first.
2. The usage of inode->i_size is wrong on 32-bit machines.
3. "Instruction at end of binary" logic is simply wrong, it doesn't handle the case when uprobe->offset > inode->i_size.
In this case "bytes" overflows, and __copy_insn() writes to the memory outside of uprobe->arch.insn.
Yes, uprobe_register() checks i_size_read(), but this file can be truncated after that. All i_size checks are racy, we do this only to catch the obvious mistakes.
Change copy_insn() to call __copy_insn() in a loop, simplify and fix the bytes/nbytes calculations.
Note: we do not care if offset + size > i_size, the users of arch_uprobe->insn can't know how many bytes were actually copied anyway. But perhaps this needs more changes.
--- kernel/events/uprobes.c | 47 +++++++++++++++++++++++------------------------ 1 files changed, 23 insertions(+), 24 deletions(-)
kernel/events/uprobes.c +21 -22
@@ -503,9 +503,8 @@ static bool consumer_del(struct uprobe *uprobe, struct uprobe_consumer *uc)return ret; } -static int -__copy_insn (struct address_space *mapping,struct file *filp,char *insn, -unsigned long nbytes,loff_t offset) +static int __copy_insn (struct address_space *mapping,struct file *filp, +void *insn,int nbytes,loff_t offset) {struct page *page;
@@ -527,28 +526,28 @@ __copy_insn(struct address_space *mapping, struct file *filp, char *insn,static int copy_insn (struct uprobe *uprobe,struct file *filp) { -struct address_space *mapping; -unsigned long nbytes; -int bytes; - - nbytes =PAGE_SIZE - (uprobe->offset & ~PAGE_MASK ); - mapping = uprobe->inode->i_mapping; +struct address_space *mapping = uprobe->inode->i_mapping; +loff_t offs = uprobe->offset; +void *insn = uprobe->arch.insn; +int size =MAX_UINSN_BYTES ; +int len, err = -EIO ; -/* Instruction at end of binary; copy only available bytes */ -if (uprobe->offset +MAX_UINSN_BYTES > uprobe->inode->i_size) - bytes = uprobe->inode->i_size - uprobe->offset; -else - bytes =MAX_UINSN_BYTES ; +do { +/* Copy only available bytes, but -EIO if nothing was read */ +if (offs >=i_size_read (uprobe->inode)) +break ; -/* Instruction at the page-boundary; copy bytes in second page */ -if (nbytes < bytes) { -int err =__copy_insn (mapping, filp, uprobe->arch.insn + nbytes, - bytes - nbytes, uprobe->offset + nbytes); + len =min_t (int, size,PAGE_SIZE - (offs & ~PAGE_MASK )); + err =__copy_insn (mapping, filp, insn, len, offs);if (err) -return err; - bytes = nbytes; - } -return __copy_insn (mapping, filp, uprobe->arch.insn, bytes, uprobe->offset); +break ; + + insn += len; + offs += len; + size -= len; + }while (size); + +return err; } static int prepare_uprobe(struct uprobe *uprobe, struct file *file,
Signature · 1 line
1.5.5.1
Oleg Nesterov
maintainerRe: [PATCH 1/1] uprobes: Fix the memory out of bound overwrite in copy_insn()
Quoted 4 lines · Oleg Nesterov
Note: we do not care if offset + size >
i_size, the users ofarch_uprobe->insncan't know how many bytes were actually copied anyway. But perhaps this needs more changes.
I guess this not is not very clear...
What I tried to say. Suppose that offset == 0 and i_size = 1. In this case we still copy MAX_UINSN_BYTES into arch.insn because the task gets the same page after page fault anyway, and we can't inform arch_uprobe_analyze_insn() that we have only read 1 byte. So this doesn't matter.
I changed this as follows:
Note: we do not care if we read extra bytes after inode->i_size if we got the valid page. This is fine because the task gets the same page after page-fault, and arch_uprobe_analyze_insn() can't know how how many bytes were actually read anyway.
I also moved the comment up, before the main loop.
Srikar, I am going to add this to my tree unless you object now. I verified that this indeed fixes the crash and the code looks "obviously correct, see below.
Oleg.
static int __copy_insn(struct address_space *mapping, struct file *filp,
void *insn, int nbytes, loff_t offset)
{
struct page *page;
if (!mapping->a_ops->readpage)
return -EIO;
/*
* Ensure that the page that has the original instruction is
* populated and in page-cache.
*/
page = read_mapping_page(mapping, offset >> PAGE_CACHE_SHIFT, filp);
if (IS_ERR(page))
return PTR_ERR(page);
copy_from_page(page, offset, insn, nbytes);
page_cache_release(page);
return 0;
}static int copy_insn(struct uprobe *uprobe, struct file *filp)
{
struct address_space *mapping = uprobe->inode->i_mapping;
loff_t offs = uprobe->offset;
void *insn = uprobe->arch.insn;
int size = MAX_UINSN_BYTES;
int len, err = -EIO;
/* Copy only available bytes, -EIO if nothing was read */
do {
if (offs >= i_size_read(uprobe->inode))
break;
len = min_t(int, size, PAGE_SIZE - (offs & ~PAGE_MASK));
err = __copy_insn(mapping, filp, insn, len, offs);
if (err)
break;
insn += len;
offs += len;
size -= len;
} while (size);
return err;
}Re: [PATCH 1/1] uprobes: Fix the memory out of bound overwrite in copy_insn()
In reply to Oleg NesterovQuoted 16 lines · Oleg Nesterov
1.
copy_insn()doesn't look very nice, all calculations are confusing and it is not immediately clear why do we read the 2nd page first.2. The usage of
inode->i_sizeis wrong on 32-bit machines.3. "Instruction at end of binary" logic is simply wrong, it doesn't handle the case when
uprobe->offset>inode->i_size.In this case "bytes" overflows, and
__copy_insn()writes to the memory outside ofuprobe->arch.insn.Yes,
uprobe_register()checksi_size_read(), but this file can be truncated after that. Alli_sizechecks are racy, we do this only to catch the obvious mistakes.
Quoted 8 lines
Change
copy_insn()to call__copy_insn()in a loop, simplify and fix the bytes/nbytes calculations.Note: we do not care if offset + size >
i_size, the users ofarch_uprobe->insncan't know how many bytes were actually copied anyway. But perhaps this needs more changes.Signed-off-by Oleg Nesterov <[email protected]>
Signature · 2 lines
Thanks and Regards Srikar Dronamraju