lkml.org 
[lkml]   [2013]   [Nov]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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;
}



\
 
 \ /
  Last update: 2013-11-08 18:01    [W:0.050 / U:0.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site