linux-kernel - Re: [PATCH v9 08/26] x86/fpu/xstate: Introduce helpers to manage the XSTATE buffer dynamically

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]

Date:   Fri, 13 Aug 2021 19:43:53 +0000
From:   "Bae, Chang Seok" <chang.seok.bae@...el.com>
To:     Borislav Petkov <bp@...en8.de>
CC:     "Lutomirski, Andy" <luto@...nel.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...nel.org" <mingo@...nel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "Brown, Len" <len.brown@...el.com>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "Macieira, Thiago" <thiago.macieira@...el.com>,
        "Liu, Jing2" <jing2.liu@...el.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v9 08/26] x86/fpu/xstate: Introduce helpers to manage the
 XSTATE buffer dynamically

On Aug 13, 2021, at 03:04, Borislav Petkov <bp@...en8.de> wrote:
> On Fri, Aug 13, 2021 at 08:04:54AM +0000, Bae, Chang Seok wrote:
>> Yes, each state offset in the non-compacted format is fixed in a machine
>> regardless of RFBM. So, simply take the size like that.
> 
> Comment above it please.
> 
> Also, why is this special case needed at all?

Without the “compacted” notion in the function name, one might call this even
with !XSAVES. But chances are very low in practice.

>> The query is intended to check whether the xstate buffer is fully expanded or
>> not -- no need to enlarge.
>> 
>> If the buffer is already the maximum, the code to retrieve XSTATE_BV, this
>> call, etc should be skipped there.  
>> 
>> If the query is moved here, I guess this call site code becomes a bit ugly.
> 
> Why does it become ugly?
> 
> You simply return early without touching the buffer at all.

Perhaps, the call site in the ptrace path becomes like this, I think:

+	if (xfeatures_mask_user_dynamic) {
+		u64 state_mask;
+
+		/* Retrieve XSTATE_BV. */
+		memcpy(&state_mask, (kbuf ?: tmpbuf) + offsetof(struct xregs_state, header),
+		       sizeof(u64));
+
+		/* Expand the xstate buffer based on the XSTATE_BV. */
+		ret = realloc_xstate_buffer(fpu, state_mask & xfeatures_mask_user_dynamic);
+		if (ret)
+			goto out;
+	}

Maybe retrieve XSTATE_BV is inevitable here. Then, it is not that ugly.

>> No, it is still pointed by fpu->state and will be freed in the exit path.
> 
> Exit path of the task?
> 
> All I see is "return -ENOMEM" and no callers of alloc_xstate_buffer()
> are calling free_xstate_buffer()...
> 
> And looking further into the patchset:
> 
> exc_device_not_available does not call free_xstate_buffer() I'm assuming
> 
> 	force_sig_fault(SIGILL, ILL_ILLOPC,..
> 
> later will cause arch_release_task_struct() to happen which will call
> free_xstate_buffer(). Yes, no?

Yes.

> I don't see any freeing in xstateregs_set() either, so what's happening
> there when it returns -ENOMEM?
> 
> I guess there we remain with the old buffer, i.e., the ptrace operation
> fails.
> 
> Am I close?

In this case, the ptracer just failed to inject some context. But the
ptracee’s context in the (old) buffer is intact. It will resume and eventually
exit. I think arch_release_task_struct()->free_xstate_buffer() will take care
of the old buffer.

Thanks,
Chang