On Thu, Nov 30, 2023 at 10:26:47AM +0100, Mickaël Salaün wrote: > On Fri, Nov 24, 2023 at 02:02:12PM +0100, Günther Noack wrote: > > On Fri, Nov 17, 2023 at 09:44:31PM +0100, Mickaël Salaün wrote: > > I don't think it is at odds with the backwards-compatibility concerns which we > > previously discussed. The synthetic groups still exist, it's just the > > "permitting LANDLOCK_ACCESS_FS_IOCTL on a file or directory" which affects a > > different set of IOCTL commands. > > It would not be a backward-compatibility issue, but it would make > LANDLOCK_ACCESS_FS_IOCTL too powerful even if we get safer/better access > rights. Furthermore, reducing the scope of LANDLOCK_ACCESS_FS_IOCTL with > new handled access rights should better inform end encourage developers > to drop LANDLOCK_ACCESS_FS_IOCTL when it is not strictly needed. > It would be useful to explain this rationale in the commit message. > See https://lore.kernel.org/all/20231020.moefooYeV9ei@xxxxxxxxxxx/ Done; I added a section to the commit message. > > > > > If the scope of LANDLOCK_ACCESS_FS_IOCTL is well documented, that should > > > > > be OK. But maybe we should rename this right to something like > > > > > LANDLOCK_ACCESS_FS_IOCTL_DEFAULT to make it more obvious that it handles > > > > > IOCTLs that are not handled by other access rights? > > > > > > > > Hmm, I'm not convinced this is a good name. It makes sense in the context of > > > > allowing "all the other ioctls" for a file or file hierarchy, but when setting > > > > LANDLOCK_ACCESS_FS_IOCTL in handled_access_fs, that flag turns off *all* ioctls, > > > > so "default" doesn't seem appropriate to me. > > > > > > It should turn off all IOCTLs that are not handled by another access > > > right. The handled access rights should be expanded the same way as the > > > allowed access rights. > > > > If you handle LANDLOCK_ACCESS_FS_IOCTL, and you don't permit anything on files > > or directories, all IOCTL commands will be forbidden, independent of what else > > is handled. > > > > The opposite is not true: > > > > If you handle LANDLOCK_ACCESS_FS_READ_FILE, and you don't handle > > LANDLOCK_ACCESS_FS_IOCTL, all IOCTL commands will happily work. > > > > So if you see it through that lens, you could say that it is only the > > LANDLOCK_ACCESS_FS_IOCTL bit in the "handled" mask which forbids any IOCTL > > commands at all. > > Handling LANDLOCK_ACCESS_FS_IOCTL does make all IOCTLs denied by > default. However, to allow IOCTLs, developers may need to allow > LANDLOCK_ACCESS_FS_IOCTL or another access right according to the > underlying semantic. > > It would be useful to add an example with your table in the > documentation, for instance with LANDLOCK_ACCESS_FS_READ_FILE (handled > or not handled, with LANDLOCK_ACCESS_FS_IOCTL or not, allowed on a file > or not...). Added an example to the documentation. —Günther
