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

[Dave Hansen]

On 11/13/2014 06:55 AM, Thomas Gleixner wrote:
> 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.

I've gone and audited the call sites and cleaned this up a bit.

>> +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. 

That was somewhat intentional with the idea that if we have a problem in
the middle of a large unmap we should attempt to complete the unmap.
But, I've changed my mind.  If we have any kind of validity issue, we
should just SIGSEGV and not attempt to keep unmapping things.  I've
updated the patch.

--
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/