lkml.org 
[lkml]   [2017]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH v2] drivers: char: mem: Fix thinkos in kmem address checks
    Date
    When borrowing the pfn_valid() check from mmap_kmem(), somebody managed
    to get physical and virtual addresses spectacularly muddled up, such
    that we've ended up with checks for one being the other. Whilst this
    does indeed prevent out-of-bounds accesses crashing, on most systems
    it also prevents the more desirable use-case of working at all ever.

    Check the *virtual* offset correctly for what it is. Furthermore, do
    so in the right place - a read or write may span multiple pages, so a
    single up-front check is insufficient. High memory accesses already
    have a similar validity check just before the copy_to_user() call, so
    just make the low memory path fully consistent with that.

    Reported-by: Jason A. Donenfeld <Jason@zx2c4.com>
    CC: stable@vger.kernel.org
    Fixes: 148a1bc84398 ("drivers: char: mem: Check {read,write}_kmem() addresses")
    Signed-off-by: Robin Murphy <robin.murphy@arm.com>
    ---

    Third time lucky... And if there's some other problem with this one then
    I guess we may as well just go ahead with Jason's revert, forget the whole
    thing, and let 'cat /dev/kmem' go back to crashing on non-x86 :)

    Robin.

    drivers/char/mem.c | 10 ++++------
    1 file changed, 4 insertions(+), 6 deletions(-)

    diff --git a/drivers/char/mem.c b/drivers/char/mem.c
    index 5bb1985ec484..6d9cc2d39d22 100644
    --- a/drivers/char/mem.c
    +++ b/drivers/char/mem.c
    @@ -381,9 +381,6 @@ static ssize_t read_kmem(struct file *file, char __user *buf,
    char *kbuf; /* k-addr because vread() takes vmlist_lock rwlock */
    int err = 0;

    - if (!pfn_valid(PFN_DOWN(p)))
    - return -EIO;
    -
    read = 0;
    if (p < (unsigned long) high_memory) {
    low_count = count;
    @@ -412,6 +409,8 @@ static ssize_t read_kmem(struct file *file, char __user *buf,
    * by the kernel or data corruption may occur
    */
    kbuf = xlate_dev_kmem_ptr((void *)p);
    + if (!virt_addr_valid(kbuf))
    + return -ENXIO;

    if (copy_to_user(buf, kbuf, sz))
    return -EFAULT;
    @@ -482,6 +481,8 @@ static ssize_t do_write_kmem(unsigned long p, const char __user *buf,
    * corruption may occur.
    */
    ptr = xlate_dev_kmem_ptr((void *)p);
    + if (!virt_addr_valid(ptr))
    + return -ENXIO;

    copied = copy_from_user(ptr, buf, sz);
    if (copied) {
    @@ -512,9 +513,6 @@ static ssize_t write_kmem(struct file *file, const char __user *buf,
    char *kbuf; /* k-addr because vwrite() takes vmlist_lock rwlock */
    int err = 0;

    - if (!pfn_valid(PFN_DOWN(p)))
    - return -EIO;
    -
    if (p < (unsigned long) high_memory) {
    unsigned long to_write = min_t(unsigned long, count,
    (unsigned long)high_memory - p);
    --
    2.10.2.dirty
    \
     
     \ /
      Last update: 2017-01-05 18:16    [W:3.914 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site