Message-ID: <25b96960-a07e-a952-5c23-786b55054126@zytor.com>
Date:   Sun, 22 Jan 2023 15:45:44 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: the x86 sysret_rip test fails on the Intel FRED architecture
Content-Language: en-US
To:     Dave Hansen <dave.hansen@intel.com>,
        "Li, Xin3" <xin3.li@intel.com>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "bp@alien8.de" <bp@alien8.de>,
        "peterz@infradead.org" <peterz@infradead.org>,
        "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>
Cc:     "x86@kernel.org" <x86@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <SA1PR11MB6734FA9139B9C9F6CC2ED123A8C59@SA1PR11MB6734.namprd11.prod.outlook.com>
 <5d4ad3e3-034f-c7da-d141-9c001c2343af@intel.com>
 <18B5DB6D-AEBD-4A67-A7B3-CE64940819B7@zytor.com>
 <SA1PR11MB673498933098295BFC7C2900A8CB9@SA1PR11MB6734.namprd11.prod.outlook.com>
 <b6e36a5c-6f5e-eda6-54ad-a0c20eb00402@intel.com>
From:   "H. Peter Anvin" <hpa@zytor.com>
In-Reply-To: <b6e36a5c-6f5e-eda6-54ad-a0c20eb00402@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 1/21/23 20:34, Dave Hansen wrote:
> On 1/21/23 19:38, Li, Xin3 wrote:
>>> However, it doesn't seem to make sense to do so to me. The current behavior is
>>> much more of an artifact than desired behavior.
>> We kind of have an agreement that %r11 = %flags after returning from the kernel.
>>
>> And the question is, is it what we want?
> 
> Can the selftest just set r11=rflags before the syscall?  The old
> syscall entry path will set r11=rflags.  The FRED path won't touch it.
> Either case will pass an r11==rflags check.

Thinking about this some more, the whole point here is to make sure that 
all syscalls work consistently, so that we don't leak a kernel 
implementation detail.

Either r11 and rcx should both be clobbered (set to %rcx and the return 
%rip respectively), or none should be.

As such, I think we want to do something like:


/* Arbitrary values */
const unsigned long r11_sentinel = 0xfeedfacedeadbeef;
const unsigned long rcx_sentinel = 0x5ca1ab1e0b57ac1e;

/* An arbitrary *valid* RFLAGS value */
const unsigned long rflags_sentinel = 0x200893;

enum regs_ok {
	REGS_ERROR  = -1,	/* Invalid register contents */
	REGS_SAVED  =  0,	/* Registers properly preserved */
	REGS_SYSRET =  1	/* Registers match syscall/sysret */
};

/*
  * Returns:
  *  0 = %rcx and %r11 preserved
  *  1 = %rcx and %r11 set to %rflags and %rip
  * -1 = %rcx and/or %r11 set to any other values
  *
  * Note that check_regs_syscall() set %rbx to the syscall return %rip.
  */
static enum regs_ok check_regs_result(unsigned long r11,
	unsigned long rcx, unsigned long rbx)
{
	if (r11 == r11_sentinel && rcx == rcx_sentinel)
		return REGS_SAVED;
	else if (r11 == rflags_sentinel && rcx == rbx)
		return REGS_SYSRET;
	else
		return REGS_ERROR;
}

static enum regs_ok check_regs_syscall(int syscall,
	unsigned long arg1, unsigned long arg2)
{

	register unsigned long r11 asm("%r11");
	unsigned long rcx, rbx, tmp;

	r11 = r11_sentinel;
	rcx = rcx_sentinel;

	asm volatile("push %3; popf; "
		     "lea 1f(%%rip),%2; "
		     "syscall; "
		     "1:"
		     : "+r" (r11), "+c" (rcx), "=b" (rbx)
		     : "g" (rflags_sentinel),
		       "a" (syscall), "D" (arg1), "S" (arg2));

	return check_regs_result(r11, rcx, rbx);
}

enum regs_ok
check_regs_getppid(void)
{
     return check_regs_syscall(__NR_getppid, 0, 0);
}


Test it out with a trivial system call like __NR_getppid which is 
extremely likely to return with SYSRET on an IDT system; check that it 
returns a nonnegative value and then save the result.

All tests from that point on should return the *same* value, including 
the signal handler etc.

I make the result-checking function separate so that it can be fed data 
from e.g. the signal stack or ptrace.

	-hpa