lkml.org 
[lkml]   [2014]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 10/11] x86, mpx: cleanup unused bound tables
On Wed, 12 Nov 2014, Dave Hansen wrote:
> +/*
> + * Get the base of bounds tables pointed by specific bounds
> + * directory entry.
> + */
> +static int get_bt_addr(struct mm_struct *mm,
> + long __user *bd_entry, unsigned long *bt_addr)
> +{
> + int ret;
> + int valid;
> +
> + if (!access_ok(VERIFY_READ, (bd_entry), sizeof(*bd_entry)))
> + return -EFAULT;
> +
> + while (1) {
> + int need_write = 0;
> +
> + pagefault_disable();
> + ret = get_user(*bt_addr, bd_entry);
> + pagefault_enable();
> + if (!ret)
> + break;
> + if (ret == -EFAULT)
> + ret = mpx_resolve_fault(bd_entry, need_write);
> + /*
> + * If we could not resolve the fault, consider it
> + * userspace's fault and error out.
> + */
> + if (ret)
> + return ret;
> + }
> +
> + valid = *bt_addr & MPX_BD_ENTRY_VALID_FLAG;
> + *bt_addr &= MPX_BT_ADDR_MASK;
> +
> + /*
> + * When the kernel is managing bounds tables, a bounds directory
> + * entry will either have a valid address (plus the valid bit)
> + * *OR* be completely empty. If we see a !valid entry *and* some
> + * data in the address field, we know something is wrong. This
> + * -EINVAL return will cause a SIGSEGV.
> + */
> + if (!valid && *bt_addr)
> + return -EINVAL;
> + /*
> + * Not present is OK. It just means there was no bounds table
> + * for this memory, which is completely OK. Make sure to distinguish
> + * this from -EINVAL, which will cause a SEGV.
> + */
> + if (!valid)
> + return -ENOENT;

So here you have the extra -ENOENT return value, but at the
direct/indirect call sites you ignore -EINVAL or everything.

> +static int mpx_unmap_tables(struct mm_struct *mm,
> + unsigned long start, unsigned long end)

> + ret = unmap_edge_bts(mm, start, end);
> + if (ret == -EFAULT)
> + return ret;

So here you ignore EINVAL despite claiming that it will cause a
SIGSEGV. So this should be:

switch (ret) {
case 0:
case -ENOENT: break;
default: return ret;
}

> + for (bd_entry = bde_start + 1; bd_entry < bde_end; bd_entry++) {
> + ret = get_bt_addr(mm, bd_entry, &bt_addr);
> + /*
> + * If we encounter an issue like a bad bounds-directory
> + * we should still try the next one.
> + */
> + if (ret)
> + continue;

You ignore all error returns.

switch (ret) {
case 0: break;
case -ENOENT: continue;
default: return ret;
}

Other than that, this all looks very reasonable now.

Thanks,

tglx


\
 
 \ /
  Last update: 2014-11-13 16:21    [W:0.053 / U:79.760 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site