On 7/3/2024 4:08 PM, KP Singh wrote: > On Thu, Jul 4, 2024 at 12:52 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: >> On Wed, Jul 3, 2024 at 6:22 PM KP Singh <kpsingh@xxxxxxxxxx> wrote: >>> On Wed, Jul 3, 2024 at 10:56 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: >>>> On Wed, Jul 3, 2024 at 12:55 PM KP Singh <kpsingh@xxxxxxxxxx> wrote: >>>>> On Wed, Jul 3, 2024 at 2:07 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: >>>>>> On Jun 29, 2024 KP Singh <kpsingh@xxxxxxxxxx> wrote: >>>>>>> LSM hooks are currently invoked from a linked list as indirect calls >>>>>>> which are invoked using retpolines as a mitigation for speculative >>>>>>> attacks (Branch History / Target injection) and add extra overhead which >>>>>>> is especially bad in kernel hot paths: >>>>> [...] >>>>> >>>>>> should fix the more obvious problems. I'd like to know if you are >>>>>> aware of any others? If not, the text above should be adjusted and >>>>>> we should reconsider patch 5/5. If there are other problems I'd >>>>>> like to better understand them as there may be an independent >>>>>> solution for those particular problems. >>>>> We did have problems with some other hooks but I was unable to dig up >>>>> specific examples though, it's been a while. More broadly speaking, a >>>>> default decision is still a decision. Whereas the intent from the BPF >>>>> LSM is not to make a default decision unless a BPF program is loaded. >>>>> I am quite worried about the holes this leaves open, subtle bugs >>>>> (security or crashes) we have not caught yet and PATCH 5/5 engineers away >>>>> the problem of the "default decision". >>>> The inode/xattr problem you originally mentioned wasn't really rooted >>>> in a "bad" default return value, it was really an issue with how the >>>> LSM hook was structured due to some legacy design assumptions made >>>> well before the initial stacking patches were merged. That should be >>>> fixed now[1] and given that the inode/xattr set/remove hooks were >>>> unique in this regard (individual LSMs were responsible for performing >>>> the capabilities checks) I don't expect this to be a general problem. >>>> >>>> There were also some issues caused by the fact that we were defining >>>> the default return value in multiple places and these values had gone >>>> out of sync in a number of hooks. We've also fixed this problem by >>>> only defining the default return value once for each hook, solving all >>>> of those problems. >>> I don't see how this solves problems or prevents any future problems >>> with side-effects. I have always been uncomfortable with an extraneous >>> function being called with a side effect ever since we merged BPF LSM >>> with default callback. We have found one bug due to this, not all the >>> bugs. >> You've got to give me something more concrete than that. If you can't >> provide any concrete examples, start with providing a basic concept >> with far more detail than just "side-effects". >> >>>> I'm not aware of any other existing problems relating to the LSM hook >>>> default values, if there are any, we need to fix them independent of >>>> this patchset. The LSM framework should function properly if the >>>> "default" values are used. >>> Patch 5 eliminates the possibilities of errors and subtle bugs all >>> together. The problem with subtle bugs is, well, they are subtle, if >>> you and I knew of the bugs, we would fix all of them, but we don't. I >>> really feel we ought to eliminate the class of issues and not just >>> whack-a-mole when we see the bugs. >> Here's the thing, I don't really like patch 5/5. To be honest, I >> don't really like a lot of this patchset. From my perspective, the >> complexity of the code is likely going to mean more maintenance >> headaches down the road, but Linus hath spoken so we're doing this >> (although "this" is still a bit undefined as far as I'm concerned). >> If you want me to merge patch 5/5 you've got to give me something real >> and convincing that can't be fixed by any other means. My current >> opinion is that you're trying to use a previously fixed bug to scare >> and/or coerce the merging of some changes I don't really want to >> merge. If you want me to take patch 5/5, you've got to give me a >> reason that is far more compelling that what you've written thus far. > Paul, I am not scaring you, I am providing a solution that saves us > from headaches with side-effects and bugs in the future. It's safer by > design. > > You say you have not reviewed it carefully, but you did ask me to move > the function from the BPF LSM layer to an LSM API, and we had a bunch > of discussion around naming in the subsequent revisions. > > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@xxxxxxxxxxxxxx/ > > My reasons are: > > 1. It's safer, no side effects, guaranteed to be not buggy. Neither > you, nor me, can guarantee that a default value will be safe in the > LSM layer. I request others (Casey, Kees) for their opinion here too. > 2. Performance, no extra function call. I want to be very careful about the comments I make on this patch set. I can't say that I trust any fix for the BPF LSM layer. My natural inclination is to isolate the fix to the area that has the problem, that is, BPF. I have a hard time accepting the notion that the implementation will really fix all possible bugs in the future. The pace at which eBPF is evolving gives me the heebee geebees when I think of it as a mechanism for implementing security modules. My biggest concern is that we may be trying too hard for perfection. I see a situation where we're not moving forward because there are two reasonable solutions and rather than running with either we're desperately looking for a compelling reason to pick one over the other. > > If you still don't like it, it's your call, I would still like to keep > most of the logic local to the BPF LSM as proposed in > https://lore.kernel.org/bpf/f7e8a16b0815d9d901e019934d684c5f@xxxxxxxxxxxxxx/ > > - KP > >> -- >> paul-moore.com
