linux-kernel - Re: the x86 sysret_rip test fails on the Intel FRED architecture

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <25b96960-a07e-a952-5c23-786b55054126@zytor.com>
Date:   Sun, 22 Jan 2023 15:45:44 -0800
From:   "H. Peter Anvin" <hpa@...or.com>
To:     Dave Hansen <dave.hansen@...el.com>,
        "Li, Xin3" <xin3.li@...el.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>
Cc:     "x86@...nel.org" <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: the x86 sysret_rip test fails on the Intel FRED architecture

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