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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 13 Nov 2014 07:29:05 -0800
From:	Dave Hansen <dave@...1.net>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	hpa@...or.com, mingo@...hat.com, x86@...nel.org,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	linux-ia64@...r.kernel.org, linux-mips@...ux-mips.org,
	qiaowei.ren@...el.com, dave.hansen@...ux.intel.com
Subject: Re: [PATCH 10/11] x86, mpx: cleanup unused bound tables

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/