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: <CAOQ4uxgakk8pW39JkjL1Up-dGZtTDn06QAQvX8p0fVZksCzA9Q@mail.gmail.com> Date: Wed, 13 Nov 2024 00:40:52 +0100 From: Amir Goldstein <amir73il@...il.com> To: Linus Torvalds <torvalds@...ux-foundation.org> Cc: Josef Bacik <josef@...icpanda.com>, kernel-team@...com, linux-fsdevel@...r.kernel.org, jack@...e.cz, brauner@...nel.org, linux-xfs@...r.kernel.org, linux-btrfs@...r.kernel.org, linux-mm@...ck.org, linux-ext4@...r.kernel.org Subject: Re: [PATCH v7 07/18] fsnotify: generate pre-content permission event on open On Tue, Nov 12, 2024 at 8:54 PM Linus Torvalds <torvalds@...ux-foundation.org> wrote: > > On Tue, 12 Nov 2024 at 09:56, Josef Bacik <josef@...icpanda.com> wrote: > > > > + /* > > + * This permission hook is different than fsnotify_open_perm() hook. > > + * This is a pre-content hook that is called without sb_writers held > > + * and after the file was truncated. > > + */ > > + return fsnotify_file_area_perm(file, MAY_OPEN, &file->f_pos, 0); > > } > > I still object to this all. > > You can't say "permission denied" after you've already truncated the > file. It's not a sane model. I complained about that earlier, it seems > that complaint was missed in the other complaints. > Not missed. I answered here: https://lore.kernel.org/linux-fsdevel/CAOQ4uxg0k4bGz6zOKS+Qt5BjEqDdUhvgG+5pLBPqSCcnQdffig@mail.gmail.com/ Starting with "...I understand why it seems stupid..." Nevertheless, we can also drop this patch for now, I don't think the post-open is a must-have hook for HSM. > Also, this whole "This permission hook is different than > fsnotify_open_perm() hook" statement is purely because > fsnotify_open_perm() itself was broken and called from the wrong place > as mentioned in the other email. > You wrote it should be called "in the open path" - that is ambiguous. pre-content hook must be called without sb_writers held, so current (in linux-next) location of fsnotify_open_perm() is not good in case of O_CREATE flag, so I am not sure where a good location is. Easier is to drop this patch. > Fix *THAT* first, then unify the two places that should *not* be > different into one single "this is the fsnotify_open" code. And that > place explicitly sets that FMODE_NOTIFY_PERM bit, and makes sure that > it does *not* set it for FMODE_NONOTIFY or FMODE_PATH cases. > > And then please work on making sure that that isn't called unless > actually required. > > The actual real "pre-content permission events" should then ONLY test > the FMODE_NOTIFY_PERM bit. Nothing else. None of this "re-use the > existing fsnotify_file() logic" stuff. Noe extra tests, no extra > logic. > > Don't make me jump through filve layers of inline functions that all > test different 'mask' bits, just to verify that the open / read / > write paths don't do something stupid. > > IOW, make it straightforward and obvious what you are doing, and make > it very clear that you're not pointlessly testing things like > FMODE_NONOTIFY when the *ONLY* thing that should be tested is whether > FMODE_NOTIFY_PERM is set. > > Please. Yes. Clear. Thanks for taking the time to look closer. and thanks for the feedback, Amir.
Powered by blists - more mailing lists