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
