linux-kernel - Re: [PATCH v2 5/9] mm: Initialize struct vm_unmapped_area

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: <ec3e377a-c0a0-4dd3-9cb9-96517e54d17e@csgroup.eu>
Date: Mon, 4 Mar 2024 18:00:43 +0000
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Kees Cook <keescook@...omium.org>, "Edgecombe, Rick P"
	<rick.p.edgecombe@...el.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"x86@...nel.org" <x86@...nel.org>, "luto@...nel.org" <luto@...nel.org>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"debug@...osinc.com" <debug@...osinc.com>, "akpm@...ux-foundation.org"
	<akpm@...ux-foundation.org>, "linux-sh@...r.kernel.org"
	<linux-sh@...r.kernel.org>, "linux-csky@...r.kernel.org"
	<linux-csky@...r.kernel.org>, "mingo@...hat.com" <mingo@...hat.com>,
	"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
	"tglx@...utronix.de" <tglx@...utronix.de>, "linux-parisc@...r.kernel.org"
	<linux-parisc@...r.kernel.org>, "loongarch@...ts.linux.dev"
	<loongarch@...ts.linux.dev>, "linux-mm@...ck.org" <linux-mm@...ck.org>,
	"linux-snps-arc@...ts.infradead.org" <linux-snps-arc@...ts.infradead.org>,
	"Liam.Howlett@...cle.com" <Liam.Howlett@...cle.com>, "hpa@...or.com"
	<hpa@...or.com>, "peterz@...radead.org" <peterz@...radead.org>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	"bp@...en8.de" <bp@...en8.de>, "linux-s390@...r.kernel.org"
	<linux-s390@...r.kernel.org>, "linux-alpha@...r.kernel.org"
	<linux-alpha@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-mips@...r.kernel.org"
	<linux-mips@...r.kernel.org>, "sparclinux@...r.kernel.org"
	<sparclinux@...r.kernel.org>, "broonie@...nel.org" <broonie@...nel.org>
Subject: Re: [PATCH v2 5/9] mm: Initialize struct vm_unmapped_area_info



Le 02/03/2024 à 02:51, Kees Cook a écrit :
> On Sat, Mar 02, 2024 at 12:47:08AM +0000, Edgecombe, Rick P wrote:
>> On Wed, 2024-02-28 at 09:21 -0800, Kees Cook wrote:
>>> I totally understand. If the "uninitialized" warnings were actually
>>> reliable, I would agree. I look at it this way:
>>>
>>> - initializations can be missed either in static initializers or via
>>>    run time initializers. (So the risk of mistake here is matched --
>>>    though I'd argue it's easier to *find* static initializers when
>>> adding
>>>    new struct members.)
>>> - uninitialized warnings are inconsistent (this becomes an unknown
>>> risk)
>>> - when a run time initializer is missed, the contents are whatever
>>> was
>>>    on the stack (high risk)
>>> - what a static initializer is missed, the content is 0 (low risk)
>>>
>>> I think unambiguous state (always 0) is significantly more important
>>> for
>>> the safety of the system as a whole. Yes, individual cases maybe bad
>>> ("what uid should this be? root?!") but from a general memory safety
>>> perspective the value doesn't become potentially influenced by order
>>> of
>>> operations, leftover stack memory, etc.
>>>
>>> I'd agree, lifting everything into a static initializer does seem
>>> cleanest of all the choices.
>>
>> Hi Kees,
>>
>> Well, I just gave this a try. It is giving me flashbacks of when I last
>> had to do a tree wide change that I couldn't fully test and the
>> breakage was caught by Linus.
> 
> Yeah, testing isn't fun for these kinds of things. This is traditionally
> why the "obviously correct" changes tend to have an easier time landing
> (i.e. adding "= {}" to all of them).
> 
>> Could you let me know if you think this is additionally worthwhile
>> cleanup outside of the guard gap improvements of this series? Because I
>> was thinking a more cowardly approach could be a new vm_unmapped_area()
>> variant that takes the new start gap member as a separate argument
>> outside of struct vm_unmapped_area_info. It would be kind of strange to
>> keep them separate, but it would be less likely to bump something.
> 
> I think you want a new member -- AIUI, that's what that struct is for.
> 
> Looking at this resulting set of patches, I do kinda think just adding
> the "= {}" in a single patch is more sensible. Having to split things
> that are know at the top of the function from the stuff known at the
> existing initialization time is rather awkward.
> 
> Personally, I think a single patch that sets "= {}" for all of them and
> drop the all the "= 0" or "= NULL" assignments would be the cleanest way
> to go.

I agree with Kees, set = {} and drop all the "something = 0;" stuff.

Christophe