Received-SPF: pass (google.com: domain of linux-kernel+bounces-29485-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240116234901.3238852-1-avagin@google.com> <30cd0be4-705f-4d63-bdad-fc57301e7eda@intel.com>
In-Reply-To: <30cd0be4-705f-4d63-bdad-fc57301e7eda@intel.com>
From: Andrei Vagin <avagin@gmail.com>
Date: Wed, 17 Jan 2024 14:30:55 -0800
Message-ID: <CANaxB-xu+zG=5_EAe4zapC5a_x4CkkDovmVX7LjiLk+E7kU75g@mail.gmail.com>
Subject: Re: [PATCH] x86/fpu: verify xstate buffer size according with
 requested features
To: Dave Hansen <dave.hansen@intel.com>
Cc: Andrei Vagin <avagin@google.com>, Thomas Gleixner <tglx@linutronix.de>, 
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, 
	Dave Hansen <dave.hansen@linux.intel.com>, LKML <linux-kernel@vger.kernel.org>, 
	x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 17, 2024 at 11:34=E2=80=AFAM Dave Hansen <dave.hansen@intel.com=
> wrote:
>
> .. adding LKML.  More context here:
>
> https://lore.kernel.org/all/20240116234901.3238852-1-avagin@google.com/
>
> On 1/16/24 15:49, Andrei Vagin wrote:
> > +     /* xstate_size has to fit all requested components. */
> > +     if (fx_sw->xstate_size !=3D fpstate->user_size) {
> > +             int min_xstate_size =3D
> > +                     xstate_calculate_size(fx_sw->xfeatures, false);
> > +
> > +             if (min_xstate_size < 0 ||
> > +                 fx_sw->xstate_size < min_xstate_size ||
> > +                 fx_sw->xstate_size > fpstate->user_size)
> > +                     goto setfx;
> > +     }
>
> The bug here is that the buffer from userspace is garbage and the (XSAVE
> XSTATE_BV) metadata doesn't match the size of the buffer.  Right?

right

>
> This proposed fix just checks another piece of user-supplied metadata
> instead: fx_sw->xstate_size.
>
> Can't userspace just provide more bad data there and end up with the
> same problem?

It can't... I would not post this change if I thought otherwise...

>
> Seems like the real problem here is that the fault_in_readable() doesn't
> match the XRSTOR.  It's going to continue to be a problem as long as we
> don't know what memory XRSTOR tried to access.  We can try all day long
> to precalculate what XRSTOR _will_ do, but that seems a bit silly

I don't understand this part. The behavior of XRSTOR is well-defined
by CPU specifications, allowing us to easily precalculate the memory it
will attempt to access. What does it mean "we don't know what memory
XRSTOR tried to access"?

xrstor restores only features that are set in fx_sw->xfeatures.

> because the CPU knows where the fault happened.  It told us in CR2 and
> all we have to do is plumb that back to fault_in_readable().

I considered this option as well, but then I decided that this approach
is better. The most important aspect is that it always rejects bad
buffers, allowing a user space to detect an issue even when a fault
isn't triggered. I believe proper handling of xrstor page faults could
be a valuable additional improvement to this change. If we detect a
fault outside of a provided buffer, we can print a warning to signal
that check_xstate_in_sigframe is incomplete.

>
> It would take a little XSTATE_OP() munging to pass something back other
> than 'err', but that doesn't seem insurmountable.
>
> Anybody have better ideas?
>