Re: [PATCH 6/6] kselftest/arm64: Check mte tagged user address in kernel

From: Catalin Marinas <catalin.marinas@arm.com>
To: Amit Kachhap <amit.kachhap@arm.com>
Cc: linux-arm-kernel@lists.infradead.org,
	linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
	Shuah Khan <shuah@kernel.org>, Will Deacon <will@kernel.org>,
	Vincenzo Frascino <Vincenzo.Frascino@arm.com>
Subject: Re: [PATCH 6/6] kselftest/arm64: Check mte tagged user address in kernel
Date: Wed, 23 Sep 2020 09:49:30 +0100	[thread overview]
Message-ID: <20200923084930.GB13434@gaia> (raw)
In-Reply-To: <d390f84d-8cd9-8646-3dab-19f62512ee21@arm.com>

On Wed, Sep 23, 2020 at 12:36:59PM +0530, Amit Kachhap wrote:
> On 9/22/20 4:11 PM, Catalin Marinas wrote:
> > On Tue, Sep 01, 2020 at 02:57:19PM +0530, Amit Daniel Kachhap wrote:
> > > +static int check_usermem_access_fault(int mem_type, int mode, int mapping)
> > > +{
> > > +	int fd, ret, i, err;
> > > +	char val = 'A';
> > > +	size_t len, read_len;
> > > +	void *ptr, *ptr_next;
> > > +	bool fault;
> > > +
> > > +	len = 2 * page_sz;
> > > +	err = KSFT_FAIL;
> > > +	/*
> > > +	 * Accessing user memory in kernel with invalid tag should fault in sync
> > > +	 * mode but may not fault in async mode as per the implemented MTE
> > > +	 * support in Arm64 kernel.
> > > +	 */
> > > +	if (mode == MTE_ASYNC_ERR)
> > > +		fault = false;
> > > +	else
> > > +		fault = true;
> > > +	mte_switch_mode(mode, MTE_ALLOW_NON_ZERO_TAG);
> > > +	fd = create_temp_file();
> > > +	if (fd == -1)
> > > +		return KSFT_FAIL;
> > > +	for (i = 0; i < len; i++)
> > > +		write(fd, &val, sizeof(val));
> > > +	lseek(fd, 0, 0);
> > > +	ptr = mte_allocate_memory(len, mem_type, mapping, true);
> > > +	if (check_allocated_memory(ptr, len, mem_type, true) != KSFT_PASS) {
> > > +		close(fd);
> > > +		return KSFT_FAIL;
> > > +	}
> > > +	mte_initialize_current_context(mode, (uintptr_t)ptr, len);
> > > +	/* Copy from file into buffer with valid tag */
> > > +	read_len = read(fd, ptr, len);
> > > +	ret = errno;
> > 
> > My reading of the man page is that errno is set only if read() returns
> > -1.
> 
> Yes. The checks should be optimized here.

It's not about optimisation but correctness. The errno man page states
that errno is only relevant if the syscall returns -1. So it may
potentially hold a stale value (e.g. EFAULT) in case of read() success
but the check below fails anyway:

> > > +	mte_wait_after_trig();
> > > +	if ((cur_mte_cxt.fault_valid == true) || ret == EFAULT || read_len < len)
> > > +		goto usermem_acc_err;

-- 
Catalin