From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 0BAEB2BDC32 for ; Tue, 30 Jun 2026 15:07:17 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782832039; cv=none; b=BnsMFvfXuLt3uP8A49MmH9oKjipiGmlvgGCgV8GlWERo8FotMe7RvVr7RfBJn90hwdejFn161F7FYji7bxQW7GW3zys3jUWpwOCFm8ziXkch3jXwtb9LcvJJ1kEuMM5DzK9t779z1ti5NVBXGR2QzOhnmwgU3xHwdShJWfIticw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1782832039; c=relaxed/simple; bh=OT0L1KfeIJmNfgfcnMydaA03DrB9zdExCI/Ns8m1d5Y=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=DmuYcsrL+NrjyOdttLibBGlPvWPDvpVZ20kwtADmGOP+IoHx+QoG9HqmOZG3HqlDhwQb2d+kz9EdOw8NOwfLo1MbMmM/zxf31zOqJhJpJDzsliC3HFXXZXrrx81SlOnYv7MxSbv55wBRA7UOIrdPuuVvhR6VotbGKxuq4WwiXOw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=foss.arm.com; spf=pass smtp.mailfrom=foss.arm.com; dkim=pass (1024-bit key) header.d=foss.arm.com header.i=@foss.arm.com header.b=Zso5WDLw; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=foss.arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=foss.arm.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=foss.arm.com header.i=@foss.arm.com header.b="Zso5WDLw" Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 001892ED2; Tue, 30 Jun 2026 08:07:13 -0700 (PDT) Received: from [10.211.55.246] (usa-sjc-mx-foss1.foss.arm.com [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D78AF3F66F; Tue, 30 Jun 2026 08:07:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=foss.arm.com; s=main; t=1782832037; bh=OT0L1KfeIJmNfgfcnMydaA03DrB9zdExCI/Ns8m1d5Y=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=Zso5WDLwdHKqBuOtL6R41HlzNz0YXvG9QzqGWz/NPS8bBPa4W6+94cLhS75dXDUtJ uukahWYZJDgeR1k3OAQKkmU5pHR3FmSa+RM8C5CAnerK5mgrHfZqqPch1oBLVJQW9t kZpwAOVY1EPcTDHZozvd8QgYGPNX/22NLGUkwLfA= Message-ID: <23f38562-1d56-4a15-b11e-293523f03fe7@foss.arm.com> Date: Mon, 29 Jun 2026 12:05:49 -0500 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 1/2] arch/x86: do not allow unlock to set bits To: "Edgecombe, Rick P" , "x86@kernel.org" , "mingo@redhat.com" , "tglx@kernel.org" , "hpa@zytor.com" , "bp@alien8.de" , "dave.hansen@linux.intel.com" Cc: "linux-kernel@vger.kernel.org" References: <20260615195156.257950-1-bill.roberts@arm.com> <0d1ba30c16d5b612d92b1b23a437384a8f24c882.camel@intel.com> Content-Language: en-US From: Bill Roberts In-Reply-To: <0d1ba30c16d5b612d92b1b23a437384a8f24c882.camel@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 6/29/26 4:44 PM, Edgecombe, Rick P wrote: > On Mon, 2026-06-15 at 14:51 -0500, Bill Roberts wrote: >> Currently, the code for handling arch_prctl for shadow stack operations >> is written to exit early on all operations but enable. However, the >> check for ARCH_SHSTK_UNLOCK is gated on a check for task != current, >> which means that if current == task, ARCH_SHSTK_UNLOCK can be used to >> set feature bits by virtue of skipping that check. >> >> This seems not as intended, and the check should first check that the >> operation is ARCH_SHSTK_UNLOCK and then check the task status to >> determine the error code. > I found this log kind of confusing. The problem being fixed is not that > ARCH_SHSTK_UNLOCK can be used when current == task, but rather that it can be > misinterpreted as ARCH_SHSTK_ENABLE. Which is allowed when current == task. So > this is a very strange ABI, but nothing is exposed. I'll make the message more clear on the v2 respin. > >> Signed-off-by: Bill Roberts >> --- >> arch/x86/kernel/shstk.c | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c >> index 0ca64900192f..664167f94acd 100644 >> --- a/arch/x86/kernel/shstk.c >> +++ b/arch/x86/kernel/shstk.c >> @@ -583,12 +583,13 @@ long shstk_prctl(struct task_struct *task, int option, unsigned long arg2) >> } >> >> /* Only allow via ptrace */ >> - if (task != current) { >> - if (option == ARCH_SHSTK_UNLOCK && IS_ENABLED(CONFIG_CHECKPOINT_RESTORE)) { >> - task->thread.features_locked &= ~features; >> - return 0; >> - } >> - return -EINVAL; >> + if (option == ARCH_SHSTK_UNLOCK) { >> + if (task == current) >> + return -EPERM; >> + if (!IS_ENABLED(CONFIG_CHECKPOINT_RESTORE)) >> + return -EINVAL; >> + task->thread.features_locked &= ~features; >> + return 0; > I think the root cause of this is the slightly odd default treatment of > ARCH_SHSTK_ENABLE. We should refactor it to be a normal case statement that > explicitly has behavior for each ARCH_SHSTK_FOO. This moves in that direction, > but leaves the code brittle. > > Ha, I had that initially, but kept the lowest line count change that fixed the issue. > Ill add that to v2. > >> } >> >> /* Do not allow to change locked features */ >