linux-kernel - Re: [PATCH 1/2] MIPS: math-emu: Update debugfs FP exception stats for certain instructions

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-next>] [day] [month] [year] [list]

Message-ID: <20171012144434.GF15235@jhogan-linux>
Date:   Thu, 12 Oct 2017 15:44:34 +0100
From:   James Hogan <james.hogan@...s.com>
To:     Aleksandar Markovic <Aleksandar.Markovic@...rk.com>
CC:     Miodrag Dinic <Miodrag.Dinic@...tec.com>,
        Paul Burton <Paul.Burton@...tec.com>,
        Petar Jovanovic <Petar.Jovanovic@...tec.com>,
        "Raghu Gandham" <Raghu.Gandham@...tec.com>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Aleksandar Markovic <Aleksandar.Markovic@...tec.com>,
        "linux-mips@...ux-mips.org" <linux-mips@...ux-mips.org>,
        Douglas Leung <Douglas.Leung@...tec.com>,
        Goran Ferenc <Goran.Ferenc@...tec.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Maciej Rozycki <Maciej.Rozycki@...tec.com>,
        Manuel Lauss <manuel.lauss@...il.com>,
        "Masahiro Yamada" <yamada.masahiro@...ionext.com>
Subject: Re: [PATCH 1/2] MIPS: math-emu: Update debugfs FP exception stats
 for certain instructions

On Thu, Oct 12, 2017 at 03:57:50PM +0200, Aleksandar Markovic wrote:
> 
> > Subject: Re: [PATCH 1/2] MIPS: math-emu: Update debugfs FP exception stats for certain instructions
> > Date: Thursday, October 12, 2017 12:17 CEST
> > From: James Hogan <james.hogan@...s.com>>@badag02.ba.imgtec.org>
> > > ...
> > > if ((ctx->fcr31 >> 5) & ctx->fcr31 & FPU_CSR_ALL_E)
> > > ...
> >
> > But just before that condition it does:
> >
> > ctx->fcr31 = (ctx->fcr31 & ~FPU_CSR_ALL_X) | rcsr;
> > I.e. it clears the X bits used in the condition, and overrides them,
> > based on rcsr, which is initialised to 0 and is only set after the
> > copcsr label and in a couple of other cases I don't think we'd be
> > hitting for MADDF.
> >
> 
> The code is odd and deceiving here. Let's see the whole "copcsr label"
> code segment: copcsr:if (ieee754_cxtest(IEEE754_INEXACT)) {  MIPS_FPU_EMU_INC_STATS(ieee754_inexact);  rcsr |= FPU_CSR_INE_X | FPU_CSR_INE_S;}if (ieee754_cxtest(IEEE754_UNDERFLOW)) {  MIPS_FPU_EMU_INC_STATS(ieee754_underflow);  rcsr |= FPU_CSR_UDF_X | FPU_CSR_UDF_S;}if (ieee754_cxtest(IEEE754_OVERFLOW)) {  MIPS_FPU_EMU_INC_STATS(ieee754_overflow);  rcsr |= FPU_CSR_OVF_X | FPU_CSR_OVF_S;}  if (ieee754_cxtest(IEEE754_INVALID_OPERATION)) {  MIPS_FPU_EMU_INC_STATS(ieee754_invalidop);  rcsr |= FPU_CSR_INV_X | FPU_CSR_INV_S;} ctx->fcr31 = (ctx->fcr31 & ~FPU_CSR_ALL_X) | rcsr;if ((ctx->fcr31 >> 5) & ctx->fcr31 & FPU_CSR_ALL_E) {  /*printk ("SIGFPE: FPU csr = %08x\n",  ctx->fcr31); */  return SIGFPE;}

Note: thats the one in fpux_emu(), not fpu_emu() which this patch
modifies. In fpu_emu() the copying of bits from rcsr to fcr32 and the
SIGFPE checking takes place outside of the switch, after other stuff can
modify rcsr.

> 
> 
> Value of rcsr will be dictated by series of invocations to ieee754_cxtest(),
> which, in fact, means that exception bits will be copied from fcr31 to rcsr.
> 
> Then, fcr31 exception bits are cleared and set to the values they had just
> before clearing.
> 
> Obviously, this will not do anything in our scenarios.
> 
> However, the patch is about correct setting of debugfs stats, and this code
> segment correctly does this.

Right, but its not going to even get to copcsr until this patch, so the
SIGFPE handling is I think fixed by this patch, i.e. it isn't just about
the stats.

> 
> May I suggest that we accept my patch as is, and if anybody for any reason
> wants to deal further with related code, this should be done in a separate
> fix/patch?

This patch fixes something, I think it should
a) be clear in the commit message what is fixed
b) be tagged for stable (though that can always be done
retrospectively).

Cheers
James

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)