linux-kernel - Re: [RESENDING PATCH] powerpc/wii: properly disable use of BATs when requested.

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

Message-ID: <20190117145552.GA2200@latitude>
Date:   Thu, 17 Jan 2019 15:55:52 +0100
From:   Jonathan Neuschäfer <j.neuschaefer@....net>
To:     Christophe Leroy <christophe.leroy@....fr>
Cc:     Jonathan Neuschäfer <j.neuschaefer@....net>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        stable@...r.kernel.org
Subject: Re: [RESENDING PATCH] powerpc/wii: properly disable use of BATs when
 requested.

On Thu, Jan 17, 2019 at 11:29:06AM +0100, Christophe Leroy wrote:
[...]
> > >   	/* MEM2 64MB@...0000000 */
> > >   	delta = wii_hole_start + wii_hole_size;
> > > +	if (__map_without_bats)
> > > +		return delta;
> > > +
> > 
> > Nothing is visibly broken without this patch, even with
> > CONFIG_DEBUG_PAGEALLOC (tested on top of v5.0-rc2), but the patch still
> > looks correct.
> 
> Obviously, CONFIG_DEBUG_PAGEALLOC cannot work without this patch.
> The purpose of CONFIG_DEBUG_PAGEALLOC is to unmap unused parts of memory so
> that any access to them will pagefault.
> 
> As this function inconditionnaly sets a BAT for the second block of RAM, any
> access to free area in the upper block will be granted without a fault.
> 
> I think you can test it by doing a kmalloc() then a kfree(), then try to
> read in that area (hopefully kmalloc() allocates memory from the top so it
> should go in the upper block). Maybe there is an LKDTM test for that.

Ah, makes sense, thanks for the explanation.

> 
> > 
> > I'd prefer the 'if' block to be before the whole delta/size calculation,
> > to make the code slightly more readable because the delta and size
> > calculations stay in one visual block. It doesn't need to happen after
> > delta is calculated.
> 
> Euh ... the function has to return 'delta', so if I put the if block before
> the calculation of delta, it means we have to calculate delta twice:

Oh, sorry, I misread the code, but you're completely right (I shouldn't
answer mails while tired).

> 
> 	if (__map_without_bats)
> 		return wii_hole_start + wii_hole_size;
> 
> 	delta = wii_hole_start + wii_hole_size;
> 
> My eyes don't really like it, so if we want to keep delta and size
> calculation together, the 'if' will go after calculation of size.

I agree.

> In anycase, this change is only really for fixing stable releases because
> this function will go away with my other serie.

ACK


Thanks,
Jonathan

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