diff options
author | Oleg Nesterov <oleg@redhat.com> | 2013-11-07 18:41:57 (GMT) |
---|---|---|
committer | Oleg Nesterov <oleg@redhat.com> | 2013-11-09 16:05:43 (GMT) |
commit | 2ded0980a6e4ae96bdd84bda66c7240967d86f3c (patch) | |
tree | 126d757f23d6fa47c04ed695eccb2b8b9e0bb176 | |
parent | 70d7f98722a7a1df1a55d6a92d0ce959c7aba9fd (diff) | |
download | linux-2ded0980a6e4ae96bdd84bda66c7240967d86f3c.tar.xz |
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 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 many bytes were actually read anyway.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
-rw-r--r-- | kernel/events/uprobes.c | 43 |
1 files changed, 21 insertions, 22 deletions
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c index 5e56950..24b7d6c 100644 --- a/kernel/events/uprobes.c +++ b/kernel/events/uprobes.c @@ -504,9 +504,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; @@ -528,28 +527,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; + /* Copy only available bytes, -EIO if nothing was read */ + do { + 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, |