From: Amit Kachhap <amit.kachhap@arm.com> To: Catalin Marinas <catalin.marinas@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 12:36:59 +0530 [thread overview] Message-ID: <d390f84d-8cd9-8646-3dab-19f62512ee21@arm.com> (raw) In-Reply-To: <20200922104123.GF15643@gaia> On 9/22/20 4:11 PM, Catalin Marinas wrote: > On Tue, Sep 01, 2020 at 02:57:19PM +0530, Amit Daniel Kachhap wrote: >> Add a testcase to check that user address with valid/invalid >> mte tag works in kernel mode. This test verifies the kernel API's >> __arch_copy_from_user/__arch_copy_to_user works by considering >> if the user pointer has valid/invalid allocation tags. >> >> In MTE sync mode a SIGSEV fault is generated if a user memory >> with invalid tag is accessed in kernel. In async mode no such >> fault occurs. > > We don't generate a SIGSEGV for faults in the uaccess routines. The > kernel simply returns less copied bytes than what was requested or -1 > and setting errno. ok. I will update in the next iteration. > > BTW, Qemu has a bug and it reports the wrong exception class (lower > DABT) for a tag check fault while in the uaccess routines, leading to > kernel panic (bad mode in synchronous abort handler). Yes I am also seeing this. > >> +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. > >> + mte_wait_after_trig(); >> + if ((cur_mte_cxt.fault_valid == true) || ret == EFAULT || read_len < len) >> + goto usermem_acc_err; >> + /* Verify same pattern is read */ >> + for (i = 0; i < len; i++) >> + if (*(char *)(ptr + i) != val) >> + break; >> + if (i < len) >> + goto usermem_acc_err; >> + >> + /* Tag the next half of memory with different value */ >> + ptr_next = (void *)((unsigned long)ptr + page_sz); >> + ptr_next = mte_insert_tags(ptr_next, page_sz); >> + if (!ptr_next) >> + goto usermem_acc_err; >> + lseek(fd, 0, 0); >> + /* Copy from file into buffer with invalid tag */ >> + read_len = read(fd, ptr, len); >> + ret = errno; >> + mte_wait_after_trig(); >> + if ((fault == true) && > > Nitpick: just use "if (fault &&), it's a bool already. ok. > >> + (cur_mte_cxt.fault_valid == true || ret == EFAULT || read_len < len)) { >> + err = KSFT_PASS; >> + } else if ((fault == false) && >> + (cur_mte_cxt.fault_valid == false && read_len == len)) { > > Same here, !fault, !cur_mte_cxt.fault_valid. ok. >
WARNING: multiple messages have this Message-ID (diff)
From: Amit Kachhap <amit.kachhap@arm.com> To: Catalin Marinas <catalin.marinas@arm.com> Cc: Shuah Khan <shuah@kernel.org>, linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org, Vincenzo Frascino <Vincenzo.Frascino@arm.com>, Will Deacon <will@kernel.org>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 6/6] kselftest/arm64: Check mte tagged user address in kernel Date: Wed, 23 Sep 2020 12:36:59 +0530 [thread overview] Message-ID: <d390f84d-8cd9-8646-3dab-19f62512ee21@arm.com> (raw) In-Reply-To: <20200922104123.GF15643@gaia> On 9/22/20 4:11 PM, Catalin Marinas wrote: > On Tue, Sep 01, 2020 at 02:57:19PM +0530, Amit Daniel Kachhap wrote: >> Add a testcase to check that user address with valid/invalid >> mte tag works in kernel mode. This test verifies the kernel API's >> __arch_copy_from_user/__arch_copy_to_user works by considering >> if the user pointer has valid/invalid allocation tags. >> >> In MTE sync mode a SIGSEV fault is generated if a user memory >> with invalid tag is accessed in kernel. In async mode no such >> fault occurs. > > We don't generate a SIGSEGV for faults in the uaccess routines. The > kernel simply returns less copied bytes than what was requested or -1 > and setting errno. ok. I will update in the next iteration. > > BTW, Qemu has a bug and it reports the wrong exception class (lower > DABT) for a tag check fault while in the uaccess routines, leading to > kernel panic (bad mode in synchronous abort handler). Yes I am also seeing this. > >> +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. > >> + mte_wait_after_trig(); >> + if ((cur_mte_cxt.fault_valid == true) || ret == EFAULT || read_len < len) >> + goto usermem_acc_err; >> + /* Verify same pattern is read */ >> + for (i = 0; i < len; i++) >> + if (*(char *)(ptr + i) != val) >> + break; >> + if (i < len) >> + goto usermem_acc_err; >> + >> + /* Tag the next half of memory with different value */ >> + ptr_next = (void *)((unsigned long)ptr + page_sz); >> + ptr_next = mte_insert_tags(ptr_next, page_sz); >> + if (!ptr_next) >> + goto usermem_acc_err; >> + lseek(fd, 0, 0); >> + /* Copy from file into buffer with invalid tag */ >> + read_len = read(fd, ptr, len); >> + ret = errno; >> + mte_wait_after_trig(); >> + if ((fault == true) && > > Nitpick: just use "if (fault &&), it's a bool already. ok. > >> + (cur_mte_cxt.fault_valid == true || ret == EFAULT || read_len < len)) { >> + err = KSFT_PASS; >> + } else if ((fault == false) && >> + (cur_mte_cxt.fault_valid == false && read_len == len)) { > > Same here, !fault, !cur_mte_cxt.fault_valid. ok. > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-09-23 7:07 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-09-01 9:27 [PATCH 0/6] kselftest: arm64/mte: Tests for user-space MTE Amit Daniel Kachhap 2020-09-01 9:27 ` Amit Daniel Kachhap 2020-09-01 9:27 ` [PATCH 1/6] kselftest/arm64: Add utilities and a test to validate mte memory Amit Daniel Kachhap 2020-09-01 9:27 ` Amit Daniel Kachhap 2020-09-21 11:36 ` Catalin Marinas 2020-09-21 11:36 ` Catalin Marinas 2020-09-21 14:18 ` Catalin Marinas 2020-09-21 14:18 ` Catalin Marinas 2020-09-21 16:43 ` Catalin Marinas 2020-09-21 16:43 ` Catalin Marinas 2020-09-01 9:27 ` [PATCH 2/6] kselftest/arm64: Verify mte tag inclusion via prctl Amit Daniel Kachhap 2020-09-01 9:27 ` Amit Daniel Kachhap 2020-09-01 9:27 ` [PATCH 3/6] kselftest/arm64: Check forked child mte memory accessibility Amit Daniel Kachhap 2020-09-01 9:27 ` Amit Daniel Kachhap 2020-09-01 9:27 ` [PATCH 4/6] kselftest/arm64: Verify all different mmap MTE options Amit Daniel Kachhap 2020-09-01 9:27 ` Amit Daniel Kachhap 2020-09-01 9:27 ` [PATCH 5/6] kselftest/arm64: Verify KSM page merge for MTE pages Amit Daniel Kachhap 2020-09-01 9:27 ` Amit Daniel Kachhap 2020-09-01 9:27 ` [PATCH 6/6] kselftest/arm64: Check mte tagged user address in kernel Amit Daniel Kachhap 2020-09-01 9:27 ` Amit Daniel Kachhap 2020-09-22 10:41 ` Catalin Marinas 2020-09-22 10:41 ` Catalin Marinas 2020-09-23 7:06 ` Amit Kachhap [this message] 2020-09-23 7:06 ` Amit Kachhap 2020-09-23 8:49 ` Catalin Marinas 2020-09-23 8:49 ` Catalin Marinas
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=d390f84d-8cd9-8646-3dab-19f62512ee21@arm.com \ --to=amit.kachhap@arm.com \ --cc=Vincenzo.Frascino@arm.com \ --cc=catalin.marinas@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-kselftest@vger.kernel.org \ --cc=shuah@kernel.org \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.