> 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!
