Re: [PATCH 10/11] x86, mpx: cleanup unused bound tables

[Thomas Gleixner]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/