linux-kernel - Re: [PATCH v3 06/12] parisc: Use initializer for 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: <32e7ccca-3bb1-4763-9b04-7a15a02a52ac@csgroup.eu>
Date: Wed, 13 Mar 2024 09:04:43 +0000
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Rick Edgecombe <rick.p.edgecombe@...el.com>, "Liam.Howlett@...cle.com"
	<Liam.Howlett@...cle.com>, "akpm@...ux-foundation.org"
	<akpm@...ux-foundation.org>, "bp@...en8.de" <bp@...en8.de>,
	"broonie@...nel.org" <broonie@...nel.org>, "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>, "debug@...osinc.com" <debug@...osinc.com>,
	"hpa@...or.com" <hpa@...or.com>, "keescook@...omium.org"
	<keescook@...omium.org>, "kirill.shutemov@...ux.intel.com"
	<kirill.shutemov@...ux.intel.com>, "luto@...nel.org" <luto@...nel.org>,
	"mingo@...hat.com" <mingo@...hat.com>, "peterz@...radead.org"
	<peterz@...radead.org>, "tglx@...utronix.de" <tglx@...utronix.de>,
	"x86@...nel.org" <x86@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>, Helge Deller <deller@....de>,
	"James E.J. Bottomley" <James.Bottomley@...senPartnership.com>,
	"linux-parisc@...r.kernel.org" <linux-parisc@...r.kernel.org>
Subject: Re: [PATCH v3 06/12] parisc: Use initializer for struct
 vm_unmapped_area_info



Le 12/03/2024 à 23:28, Rick Edgecombe a écrit :
> Future changes will need to add a new member to struct
> vm_unmapped_area_info. This would cause trouble for any call site that
> doesn't initialize the struct. Currently every caller sets each member
> manually, so if new members are added they will be uninitialized and the
> core code parsing the struct will see garbage in the new member.
> 
> It could be possible to initialize the new member manually to 0 at each
> call site. This and a couple other options were discussed, and a working
> consensus (see links) was that in general the best way to accomplish this
> would be via static initialization with designated member initiators.
> Having some struct vm_unmapped_area_info instances not zero initialized
> will put those sites at risk of feeding garbage into vm_unmapped_area() if
> the convention is to zero initialize the struct and any new member addition
> misses a call site that initializes each member manually.
> 
> It could be possible to leave the code mostly untouched, and just change
> the line:
> struct vm_unmapped_area_info info
> to:
> struct vm_unmapped_area_info info = {};
> 
> However, that would leave cleanup for the members that are manually set
> to zero, as it would no longer be required.
> 
> So to be reduce the chance of bugs via uninitialized members, instead
> simply continue the process to initialize the struct this way tree wide.
> This will zero any unspecified members. Move the member initializers to the
> struct declaration when they are known at that time. Leave the members out
> that were manually initialized to zero, as this would be redundant for
> designated initializers.
> 
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@...el.com>

Reviewed-by: Christophe Leroy <christophe.leroy@...roup.eu>


> Acked-by: Helge Deller <deller@....de>
> Cc: "James E.J. Bottomley" <James.Bottomley@...senPartnership.com>
> Cc: Helge Deller <deller@....de>
> Cc: linux-parisc@...r.kernel.org
> Link: https://lore.kernel.org/lkml/202402280912.33AEE7A9CF@keescook/#t
> Link: https://lore.kernel.org/lkml/j7bfvig3gew3qruouxrh7z7ehjjafrgkbcmg6tcghhfh3rhmzi@wzlcoecgy5rs/
> ---
> v3:
>   - Fixed spelling errors in log
>   - Be consistent about field vs member in log
> 
> Hi,
> 
> This patch was split and refactored out of a tree-wide change [0] to just
> zero-init each struct vm_unmapped_area_info. The overall goal of the
> series is to help shadow stack guard gaps. Currently, there is only one
> arch with shadow stacks, but two more are in progress. It is compile tested
> only.
> 
> There was further discussion that this method of initializing the structs
> while nice in some ways has a greater risk of introducing bugs in some of
> the more complicated callers. Since this version was reviewed my arch
> maintainers already, leave it as was already acknowledged.
> 
> Thanks,
> 
> Rick
> 
> [0] https://lore.kernel.org/lkml/20240226190951.3240433-6-rick.p.edgecombe@intel.com/
> ---
>   arch/parisc/kernel/sys_parisc.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/parisc/kernel/sys_parisc.c b/arch/parisc/kernel/sys_parisc.c
> index 98af719d5f85..f7722451276e 100644
> --- a/arch/parisc/kernel/sys_parisc.c
> +++ b/arch/parisc/kernel/sys_parisc.c
> @@ -104,7 +104,9 @@ static unsigned long arch_get_unmapped_area_common(struct file *filp,
>   	struct vm_area_struct *vma, *prev;
>   	unsigned long filp_pgoff;
>   	int do_color_align;
> -	struct vm_unmapped_area_info info;
> +	struct vm_unmapped_area_info info = {
> +		.length = len
> +	};
>   
>   	if (unlikely(len > TASK_SIZE))
>   		return -ENOMEM;
> @@ -139,7 +141,6 @@ static unsigned long arch_get_unmapped_area_common(struct file *filp,
>   			return addr;
>   	}
>   
> -	info.length = len;
>   	info.align_mask = do_color_align ? (PAGE_MASK & (SHM_COLOUR - 1)) : 0;
>   	info.align_offset = shared_align_offset(filp_pgoff, pgoff);
>   
> @@ -160,7 +161,6 @@ static unsigned long arch_get_unmapped_area_common(struct file *filp,
>   		 */
>   	}
>   
> -	info.flags = 0;
>   	info.low_limit = mm->mmap_base;
>   	info.high_limit = mmap_upper_limit(NULL);
>   	return vm_unmapped_area(&info);