linux-kernel - Re: [GIT PULL] lsm/lsm-pr-20240105

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]

Date: Wed, 10 Jan 2024 13:20:39 -0800
From: Casey Schaufler <casey@...aufler-ca.com>
To: Paul Moore <paul@...l-moore.com>,
 Linus Torvalds <torvalds@...ux-foundation.org>
Cc: linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org,
 Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [GIT PULL] lsm/lsm-pr-20240105

On 1/10/2024 12:58 PM, Paul Moore wrote:
> On Wed, Jan 10, 2024 at 3:22 PM Linus Torvalds
> <torvalds@...ux-foundation.org> wrote:
>> On Wed, 10 Jan 2024 at 11:54, Paul Moore <paul@...l-moore.com> wrote:
>>> Thanks for pulling the changes, I'm sorry the syscall table entries
>>> for the LSM syscalls were not how you want to see them, but I'm more
>>> than a little confused as to what exactly we did wrong here.
>> Look at commit 5f42375904b0 ("LSM: wireup Linux Security Module
>> syscalls") and notice for example this:
>>
>>   --- a/arch/x86/entry/syscalls/syscall_64.tbl
>>   +++ b/arch/x86/entry/syscalls/syscall_64.tbl
>>   @@ -378,6 +378,9 @@
>>    454    common  futex_wake              sys_futex_wake
>>    455    common  futex_wait              sys_futex_wait
>>    456    common  futex_requeue           sys_futex_requeue
>>   +457    common  lsm_get_self_attr       sys_lsm_get_self_attr
>>   +458    common  lsm_set_self_attr       sys_lsm_set_self_attr
>>   +459    common  lsm_list_modules        sys_lsm_list_modules
>>
>> Ok, fine - you added your new system calls to the end of the table.
>> Sure, I ended up having to fix them up because the "end of the table"
>> was different by the time I merged your tree, but that wasn't the
>> problem.
>>
>> The problem is here - in the same commit:
>>
>>   --- a/tools/perf/arch/x86/entry/syscalls/syscall_64.tbl
>>   +++ b/tools/perf/arch/x86/entry/syscalls/syscall_64.tbl
>>   @@ -375,6 +375,9 @@
>>    451    common  cachestat               sys_cachestat
>>    452    common  fchmodat2               sys_fchmodat2
>>    453    64      map_shadow_stack        sys_map_shadow_stack
>>   +454    common  lsm_get_self_attr       sys_lsm_get_self_attr
>>   +455    common  lsm_set_self_attr       sys_lsm_set_self_attr
>>   +456    common  lsm_list_modules        sys_lsm_list_modules
>>
>> note how you updated the tools copy WITH THE WRONG NUMBERS!
>>
>> You just added them at the end of the table again, and just
>> incremented the numbers, but that was complete nonsense, because the
>> numbers didn't actually match the real system call numbers, because
>> that tools table hadn't been updated for new system calls - because it
>> hadn't needed them.
>>
>> Yeah, our tooling header duplication is annoying, but the old
>> situation where the tooling just used various kernel headers directly
>> and would randomly break when kernel changes were made was even worse.
>>
>> End result: avoid touching the tooling headers - and if you have to,
>> you need to *think* about it.
> Thanks for the explanation, when I read your comment about "tools" I
> was thinking of whatever tooling transforms the arch/**/*.tbl file and
> not the tools/perf directory.  I should have caught the tools/perf
> mismatch when reviewing the patches from Casey, but I didn't, I'm
> sorry.  My guess is that my mind was just in the "use the next three
> numbers" due to the lack of syscall number sync across architectures,
> but who knows.  My mistake, I'll make sure it doesn't happen again.

No, It's my mistake. I could have asked for help when I found my head
spinning from the syscall numbering scheme and its various implications.
I will have a look at how it might be improved. Sorry to have mucked it
up, and thank you for the explanation.