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
| ||
|
Date: Fri, 8 Nov 2013 17:24:15 +0100 From: Oleg Nesterov <oleg@...hat.com> To: Ingo Molnar <mingo@...nel.org> Cc: Ingo Molnar <mingo@...e.hu>, Ananth N Mavinakayanahalli <ananth@...ibm.com>, David Long <dave.long@...aro.org>, Srikar Dronamraju <srikar@...ux.vnet.ibm.com>, linux-kernel@...r.kernel.org Subject: Re: [PATCH 1/1] uprobes: Fix the memory out of bound overwrite in copy_insn() On 11/07, Oleg Nesterov wrote: > > 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. 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; } -- 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