Linux-man Archive on lore.kernel.org
 help / color / Atom feed
From: Keno Fischer <keno@juliacomputing.com>
To: Denys Vlasenko <vda.linux@googlemail.com>
Cc: Michael Kerrisk <mtk.manpages@gmail.com>,
	linux-man <linux-man@vger.kernel.org>,
	Andy Lutomirski <luto@kernel.org>,
	Dave Hansen <dave.hansen@intel.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, X86 ML <x86@kernel.org>,
	"H. Peter Anvin" <hpa@zytor.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Andi Kleen <andi@firstfloor.org>
Subject: Re: [PATCH] ptrace.2: Describe PTRACE_SET/GETREGSET on NT_X86_XSTATE
Date: Tue, 19 May 2020 18:46:24 -0400
Message-ID: <CABV8kRxvt0EKb3VF0JKGR43Ozaujj9PB3cMx6K7e_qLoFrqemA@mail.gmail.com> (raw)
In-Reply-To: <CAK1hOcNGn7BwNNMkZZnm-d9OepuF+3UPxjL6u_6xjxFONENMKQ@mail.gmail.com>

> > > Further details on the layout of the compacted xsave
> > > +area may be found in the Intel architecture manual, but note that the xsave
> > > +buffer returned from ptrace will have its XCOMP_BV set to 0.
>
> I propose s/will have its XCOMP_BV set to 0/may have its XCOMP_BV set to 0/
> (future-proofing for the case that kernel may change this behavior).

Sounds good, though I would hope that this behavior change comes with some
flag to enable it :).

> > > +.IP 3.
> > > +For the given state component of interest, check the corresponding bit
> > > +in the xsave header's XSTATE_BV bitfield.
>
> "...(the 64-bit field at 512 byte offset in the area)".

applied. Will send out with v2 once I have all the comments in v1.

> > If this bit is zero, the corresponding
> > > +component is in initial state and was not written to the buffer (i.e. the kernel
> > > +does not touch the memory corresponding to this state component at all,
>
> We probably can not guarantee these "untouched" parts
> retain their contents before syscall, it's possible future kernels may zero-fill
> it - I propose to not over-promise here. How about "the contents is undefined,
> it may remain untouched, or be filled with dummy data"?

Sounds reasonable. Applied.

> > > +
> > > +Thus, to obtain an xsave area that may be set back to the tracee, all unused
> > > +state components must first be re-set to the correct initial state for the
> > > +corresponding state component, and the XSTATE_BV bitfield must subsequently
> > > +be adjusted to match the kernel xstate component bitmask (obtained as
> > > +described above).
>
> The above paragraph needs a better wording. Are you saying the following? -
>
> "If a state component is not saved (its XSTATE_BV bit is zero) but you
> want to modify corresponding registers in the tracee, you need to set
> this bit to 1 and initialize the component to the desired state."

Kind of, what I want to get across is a warning that the following pattern:

struct iov = { ... };
ptrace(PTRACE_GETREGSET, pid1, NT_X86_XSTATE, &iov);
ptrace(PTRACE_SETREGSET, pid2, NT_X86_XSTATE, &iov);

will not necessarily result in pid1 and pid2 having identical register states.
If a state component was in its initial state in pid1, the XSTATE_BV
bit will be cleared, resulting in the registers in pid2 not being modified.
Perhaps the easiest thing is just to include this example as well as
some pseudo-code like the following:

struct user_xstateregs *buffer = ...;
struct iov = { iov_base=(uint8_t*)buffer, iov_len=... };
ptrace(PTRACE_GETREGSET, pid1, NT_X86_XSTATE, &iov);
uint64_t kernel_xmask = buffer->i387.xstate_fx_sw[0];
uint64_t active_mask = buffer->header.xfeatures;
for (int i = 0; i < XFEATURES_MAX; ++i) {
    uint64_t bit = (uint64_t)1 << i;
    if ((kernel_mask & big) && !(active_mask & bit)) {
        reset_state_component(buffer, i);
    }
}
ptrace(PTRACE_SETREGSET, pid2, NT_X86_XSTATE, &iov);

to show how to make sure that pid2 ends up with the same register
state as pid1.

  reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-18  3:00 Keno Fischer
2020-05-19 20:44 ` Michael Kerrisk (man-pages)
2020-05-19 21:29   ` Denys Vlasenko
2020-05-19 22:46     ` Keno Fischer [this message]
2020-05-20 10:03       ` Denys Vlasenko
2020-05-20  1:19 ` Andi Kleen
2020-05-20  3:30   ` Keno Fischer
2020-05-20  5:08     ` Michael Kerrisk (man-pages)
2020-05-20 13:56     ` Andi Kleen

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=CABV8kRxvt0EKb3VF0JKGR43Ozaujj9PB3cMx6K7e_qLoFrqemA@mail.gmail.com \
    --to=keno@juliacomputing.com \
    --cc=andi@firstfloor.org \
    --cc=bp@alien8.de \
    --cc=dave.hansen@intel.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=hpa@zytor.com \
    --cc=linux-man@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mingo@redhat.com \
    --cc=mtk.manpages@gmail.com \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    --cc=vda.linux@googlemail.com \
    --cc=x86@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: link

Linux-man Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-man/0 linux-man/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-man linux-man/ https://lore.kernel.org/linux-man \
		linux-man@vger.kernel.org
	public-inbox-index linux-man

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-man


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git