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: <CANaxB-xu+zG=5_EAe4zapC5a_x4CkkDovmVX7LjiLk+E7kU75g@mail.gmail.com>
Date: Wed, 17 Jan 2024 14:30:55 -0800
From: Andrei Vagin <avagin@...il.com>
To: Dave Hansen <dave.hansen@...el.com>
Cc: Andrei Vagin <avagin@...gle.com>, Thomas Gleixner <tglx@...utronix.de>, 
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, LKML <linux-kernel@...r.kernel.org>, 
	x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH] x86/fpu: verify xstate buffer size according with
 requested features

On Wed, Jan 17, 2024 at 11:34 AM Dave Hansen <dave.hansen@...el.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 != fpstate->user_size) {
> > +             int min_xstate_size =
> > +                     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?
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ