linux-kernel - Re: [PATCH] mm: move the check of READ_IMPLIES_EXEC out of do

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]

Message-ID: <CAHQche_92iaqxWK7WKDnEDkCRV-r2HXL2xuRa7b2NBaLhUg7-Q@mail.gmail.com>
Date: Wed, 25 Sep 2024 19:36:59 +0800
From: Shu Han <ebpqwerty472123@...il.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: akpm@...ux-foundation.org, paul@...l-moore.com, jmorris@...ei.org, 
	serge@...lyn.com, Liam.Howlett@...cle.com, vbabka@...e.cz, linux-mm@...ck.org, 
	linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org, 
	Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH] mm: move the check of READ_IMPLIES_EXEC out of do_mmap()

> No need to be sorry! :) Sorry if I sound harsh here - it's more a case of
> trying to be as clear as I can be as that is the best approach for everyone
> I think.

> This code is sensitive, so we have to super careful!

Thanks a lot! :)

> I would disagree it's down to taste, I noted on the move the check to
> do_mmap() series a number of issues and concerns, to me that seems
> unworkable in it's current form, the locking thing is fatal for instance.

> What you link to there seems to be neither approach (I didn't read your
> second series though as that needs an RFC tag)? I mean I think perhaps what
> you are doing there is the best _first step_ - simply add the checks in
> each of the callsites that you feel are missing them.

> This is the least controversial way and then allows maintainers of the
> callers to assess whether they intended for that.

> If then you end up wtih _all_ callers doing this check, we can take another
> look at possibly bringing it into do_mmap() but we would absolutely have to
> ensure it was done correctly, however.

> 1. (If you haven't already) Submit a series that adds patches to add checks
>    at call sites that don't already check.

> 2. If these are accepted at _all_ callsites, revisit the do_mmap() change,
>    properly accounting for locks (I can help with this).

In fact, "mm: move the check of READ_IMPLIES_EXEC out of do_mmap()" does
not have the locking issue. These two patches are quite different. This is
also the modification I recommended, while another modification was
suggested by LSM maintainers(Perhaps I need to add suggested-by? But
that was mentioned in a non-public security mailing list, and I'm not sure
if it's appropriate.).

The __core__ problem is "no LSM hook" +
"have logic about READ_IMPLIES_EXEC". Removing one of them is OK.

The "mm: move security_file_mmap() back into do_mmap()" fixes this by
adding a LSM hook. The requirement to call LSM hooks comes from the
LSM modules, _not these call sites_. The issue for locks also comes
from the specific implementation of LSM modules. So I send patches
to LSM maintainers at the same time.

The "mm: move the check of READ_IMPLIES_EXEC out of do_mmap()" fixes this
by removing the logic about READ_IMPLIES_EXEC that is not needed. So no
locking issues there(no changes to LSM). This will result in some minor
behavioral changes for call sites mentioned in the patch. Unfortunately,
due to this logic being placed in a single function do_mmap now, it is
impossible to confirm it through patches one by one before change the mm
module. Fortunately, these changes should clearly be fine, and here are
the reasons(more specific versions):

fs/aio.c, mm/util.c, ipc/shm.c: no changes
arch/x86/kernel/shstk.c: Shadow Stack is stack only store return
addresses, adding execute permission to shadow stack is never
required.
mm/mmap.c: in the history, remap_file_pages won't care about the
READ_IMPLIES_EXEC. this side effect is introduced in the emulated
version, after the deprecated mark exists. The patch only removes the
side effects introduced. And this(mm) is the module. :)

BTW, The link is the _first step_ in required(if the check is missing in
that call site, there will be a bug) call sites, which has been done.

> I do feel we need to better document these functions, so I will add
> comments. I see you did so as part of your other series, but think maybe we
> need to expand this and possibly rename both and add some asserts... it's
> on the todo list!

Perhaps adding sufficient comments is also a completely appropriate method
as another alternative.

Thanks for your kind review!