| 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
| ||
|
Message-ID: <CABi2SkUn5pTwU9yc+6UCOXW_Q1CoFE4t1BdGQs1wq73TLvGW5w@mail.gmail.com>
Date: Wed, 13 Nov 2024 13:38:32 -0800
From: Jeff Xu <jeffxu@...omium.org>
To: "Liam R. Howlett" <Liam.Howlett@...cle.com>, Jeff Xu <jeffxu@...omium.org>,
akpm@...ux-foundation.org, keescook@...omium.org, jannh@...gle.com,
torvalds@...ux-foundation.org, adhemerval.zanella@...aro.org, oleg@...hat.com,
linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org,
linux-mm@...ck.org, jorgelo@...omium.org, sroettger@...gle.com,
ojeda@...nel.org, adobriyan@...il.com, anna-maria@...utronix.de,
mark.rutland@....com, linus.walleij@...aro.org, Jason@...c4.com,
deller@....de, rdunlap@...radead.org, davem@...emloft.net, hch@....de,
peterx@...hat.com, hca@...ux.ibm.com, f.fainelli@...il.com, gerg@...nel.org,
dave.hansen@...ux.intel.com, mingo@...nel.org, ardb@...nel.org,
mhocko@...e.com, 42.hyeyoo@...il.com, peterz@...radead.org, ardb@...gle.com,
enh@...gle.com, rientjes@...gle.com, groeck@...omium.org,
lorenzo.stoakes@...cle.com
Subject: Re: [RFC PATCH v2 1/1] exec: seal system mappings
On Mon, Nov 11, 2024 at 2:35 PM Liam R. Howlett <Liam.Howlett@...cle.com> wrote:
> > To make modulization better, I can do below adjustment:
> > if (seal_system_mapping_enabled()) <-- implemented by fs/exec.c
> > add_vm_sealed() <- keep in include/linux/mm.h
> >
> > However, if you have a strong opinion on this, I could move the
> > parsing logic to mm/mseal.
>
> Yes, please move the parsing logic together with the other mseal code.
>
Sure
>
> I am not sure what one you are renaming or what these functions would
> do. I think you need to look at your overall design and try to fit it
> into what exists instead of putting a call in a wrapper function
> (_install_special_mapping) to alter an argument.
>
Please see V3.
> >
> > > > > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > > > > index 57fd5ab2abe7..d4717e34a60d 100644
> > > > > > --- a/mm/mmap.c
> > > > > > +++ b/mm/mmap.c
> > > > > > @@ -2133,6 +2133,7 @@ struct vm_area_struct *_install_special_mapping(
> > > > > > unsigned long addr, unsigned long len,
> > > > > > unsigned long vm_flags, const struct vm_special_mapping *spec)
> > > > > > {
> > > > > > + update_seal_exec_system_mappings(&vm_flags);
> > > > > > return __install_special_mapping(mm, addr, len, vm_flags, (void *)spec,
> > > > > > &special_mapping_vmops);
> > > > >
> > > > > If you were to return a flag, you could change the vm_flags argument to
> > > > > vm_flags | mseal_flag()
> > > > >
> > > > passing pointer seems to be the most efficient way.
> > >
> > > I disagree. Here is the godbolt.org output for gcc x86-64 14.2 of your
> > > code (with some added #defines to make it compile)
> > >
> > > seal_system_mappings:
> > > .long 1
> > > seal_system_mappings_enabled:
> > > push rbp
> > > mov rbp, rsp
> > > mov eax, DWORD PTR seal_system_mappings[rip]
> > > cmp eax, 1
> > > jne .L2
> > > mov eax, 1
> > > jmp .L3
> > > .L2:
> > > mov eax, 0
> > > .L3:
> > > pop rbp
> > > ret
> > > update_seal_exec_system_mappings:
> > > push rbp
> > > mov rbp, rsp
> > > sub rsp, 8
> > > mov QWORD PTR [rbp-8], rdi
> > > mov rax, QWORD PTR [rbp-8]
> > > mov rax, QWORD PTR [rax]
> > > and eax, 2
> > > test rax, rax
> > > jne .L6
> > > call seal_system_mappings_enabled
> > > test al, al
> > > je .L6
> > > mov rax, QWORD PTR [rbp-8]
> > > mov rax, QWORD PTR [rax]
> > > or rax, 2
> > > mov rdx, rax
> > > mov rax, QWORD PTR [rbp-8]
> > > mov QWORD PTR [rax], rdx
> > > .L6:
> > > nop
> > > leave
> > > ret
> > > main:
> > > push rbp
> > > mov rbp, rsp
> > > sub rsp, 16
> > > mov QWORD PTR [rbp-8], 0
> > > lea rax, [rbp-8]
> > > mov rdi, rax
> > > call update_seal_exec_system_mappings
> > > mov rax, QWORD PTR [rbp-8]
> > > leave
> > > ret
> > >
> > > ----- 48 lines -----
> > > Here is what I am suggesting to do with replacing the passing of a
> > > pointer with a concise "vm_flags | mseal_exec_flags()" (with the same
> > > added #defines to make it compile)
> > >
> > > seal_system_mappings:
> > > .long 1
> > > mseal_exec_flags:
> > > push rbp
> > > mov rbp, rsp
> > > mov eax, DWORD PTR seal_system_mappings[rip]
> > > cmp eax, 1
> > > jne .L2
> > > mov eax, 2
> > > jmp .L3
> > > .L2:
> > > mov eax, 0
> > > .L3:
> > > pop rbp
> > > ret
> > > main:
> > > push rbp
> > > mov rbp, rsp
> > > sub rsp, 16
> > > mov QWORD PTR [rbp-8], 0
> > > call mseal_exec_flags
> > > mov edx, eax
> > > mov rax, QWORD PTR [rbp-8]
> > > or eax, edx
> > > leave
> > > ret
> > >
> > > ----- 26 lines -----
> > >
> > > So as you can see, there are less instructions in my version; there are
> > > 47.92% less lines of assembly.
> > >
> > vm_flags already run out of space in 32 bit, sooner or later we will
> > need to change that to *** a struct ***, passing address will be
> > becoming necessary with struct. Since this is not a performance
> > sensitive code path, 3 or 4 times during execve(), I think it would be
> > good to start here.
>
> No.
>
> I have pointed out why this is not 'the most efficient way' and you are
> now switching your argument to 'this will be needed in the future'.
>
> Please fix this.
>
Sure, updated in V3
Powered by blists - more mailing lists