On Sat, Mar 2, 2024 at 8:17 AM Rick Edgecombe
<rick.p.edgecombe@xxxxxxxxx> wrote:
>
> 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 field
> manually, so if new fields are added they will be unitialized and the core
> code parsing the struct will see garbage in the new field.
>
> It could be possible to initialize the new field manually to 0 at each
> call site. This and a couple other options were discussed, and the
> consensus (see links) was that in general the best way to accomplish this
> would be via static initialization with designated field 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 field addition
> misses a call site that initializes each field 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 fields that are manually set
> to zero, as it would no longer be required.
>
> So to be reduce the chance of bugs via uninitialized fields, instead
> simply continue the process to initialize the struct this way tree wide.
> This will zero any unspecified members. Move the field initializers to the
> struct declaration when they are known at that time. Leave the fields out
> that were manually initialized to zero, as this would be redundant for
> designated initializers.
>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> Cc: Guo Ren <guoren@xxxxxxxxxx>
> Cc: linux-csky@xxxxxxxxxxxxxxx
> Link: https://lore.kernel.org/lkml/202402280912.33AEE7A9CF@keescook/#t
> Link: https://lore.kernel.org/lkml/j7bfvig3gew3qruouxrh7z7ehjjafrgkbcmg6tcghhfh3rhmzi@wzlcoecgy5rs/
> ---
> 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 0day tested
> only.
>
> Thanks,
>
> Rick
>
> [0] https://lore.kernel.org/lkml/20240226190951.3240433-6-rick.p.edgecombe@xxxxxxxxx/
> ---
> arch/csky/abiv1/mmap.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/csky/abiv1/mmap.c b/arch/csky/abiv1/mmap.c
> index 6792aca49999..7f826331d409 100644
> --- a/arch/csky/abiv1/mmap.c
> +++ b/arch/csky/abiv1/mmap.c
> @@ -28,7 +28,12 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr,
> struct mm_struct *mm = current->mm;
> struct vm_area_struct *vma;
> int do_align = 0;
> - struct vm_unmapped_area_info info;
> + struct vm_unmapped_area_info info = {
> + .length = len,
> + .low_limit = mm->mmap_base,
> + .high_limit = TASK_SIZE,
> + .align_offset = pgoff << PAGE_SHIFT
> + };
>
> /*
> * We only need to do colour alignment if either the I or D
> @@ -61,11 +66,6 @@ arch_get_unmapped_area(struct file *filp, unsigned long addr,
> return addr;
> }
>
> - info.flags = 0;
> - info.length = len;
> - info.low_limit = mm->mmap_base;
> - info.high_limit = TASK_SIZE;
> info.align_mask = do_align ? (PAGE_MASK & (SHMLBA - 1)) : 0;
> - info.align_offset = pgoff << PAGE_SHIFT;
> return vm_unmapped_area(&info);
> }
> --
> 2.34.1
>
LGTM, that's equivalent.
Reviewed-by: Guo Ren <guoren@xxxxxxxxxx>
--
Best Regards
Guo Ren
