On Mon, Sep 11, 2023 at 05:25:34PM +0200, Mickaël Salaün wrote:
> On Mon, Sep 11, 2023 at 12:02:46PM +0200, Günther Noack wrote:
> > Hello!
> >
> > On Mon, Sep 04, 2023 at 08:08:29PM +0200, Mickaël Salaün wrote:
> > > On Sat, Sep 02, 2023 at 01:53:57PM +0200, Günther Noack wrote:
> > > > On Sat, Aug 26, 2023 at 08:26:30PM +0200, Mickaël Salaün wrote:
> > > > > On Fri, Aug 25, 2023 at 06:50:29PM +0200, Mickaël Salaün wrote:
> > > > > > On Fri, Aug 25, 2023 at 05:03:43PM +0200, Günther Noack wrote:
> > > > > > > (In fact, it seems to me almost like FIOQSIZE might rather be missing a security
> > > > > > > hook check for one of the "getting file attribute" hooks?)
> > > > > > >
> > > > > > > So basically, the implementation that I currently ended up with is:
> > > > > > >
> > > > > >
> > > > > > Before checking these commands, we first need to check that the original
> > > > > > domain handle LANDLOCK_ACCESS_FS_IOCTL. We should try to pack this new
> > > > > > bit and replace the file's allowed_access field (see
> > > > > > landlock_add_fs_access_mask() and similar helpers in the network patch
> > > > > > series that does a similar thing but for ruleset's handle access
> > > > > > rights), but here is the idea:
> > > > > >
> > > > > > if (!landlock_file_handles_ioctl(file))
> > > > > > return 0;
> > > > > >
> > > > > > > switch (cmd) {
> > > > > > /*
> > > > > > * Allows file descriptor and file description operations (see
> > > > > > * fcntl commands).
> > > > > > */
> > > > > > > case FIOCLEX:
> > > > > > > case FIONCLEX:
> > > > > > > case FIONBIO:
> > > > > > > case FIOASYNC:
> > > > > >
> > > > > > > case FIOQSIZE:
> > > > >
> > > > > We need to handle FIOQSIZE as done by do_vfs_ioctl: add the same i_mode
> > > > > checks. A kselftest test should check that ENOTTY is returned according
> > > > > to the file type and the access rights.
> > > >
> > > > It's not clear to me why we would need to add the same i_mode checks for
> > > > S_ISDIR() || S_ISREG() || S_ISLNK() there? If these checks in do_vfs_ioctl()
> > > > fail, it returns -ENOTTY. Is that not an appropriate error already?
> > >
> > > The LSM IOCTL hook is called before do_vfs_ioctl(), and I think that
> > > Landlock should not change the error codes according to the error types
> > > (i.e. return ENOTTY when the inode is something else than a directory,
> > > regular file, or symlink). Indeed, I think it's valuable to not blindly
> > > return EACCES when the IOCTL doesn't make sense for this file type. This
> > > should help user space handle meaningful error messages, inconsistent
> > > requests, and fallback mechanisms. Tests should check these return
> > > codes, see the network patch series (and especially the latest reviews
> > > and changes) that takes care of this kind of error codes compatibility.
> > >
> > > We could also return 0 (i.e. accept the request) and rely on the
> > > do_vfs_ioctl() check to return ENOTTY, but this is unnecessary risky
> > > from an access control point of view. Let's directly return ENOTTY as a
> > > safeguard (which BTW also avoids some useless CPU instructions) and test
> > > this behavior.
> > >
> > > I think an access control mechanism, and especially Landlock, should be
> > > as discreet as possible, and help developers quickly debug issues (and
> > > avoid going through the access control layer if it doesn't make sense).
> > > I think error ordering like this could be useful but I'd like to get
> > > other point of views.
> >
> > I see what you mean now, OK.
> >
> > Another option, btw, would be to return ENOTTY generally when Landlock denies an
> > IOCTL attempt, instead of EACCES, as I have previously suggested in
> > https://lore.kernel.org/all/ZNpnrCjYqFoGkwyf@xxxxxxxxxx/ -- should we maybe just
> > do that instead?
> >
> > I believe that users of ioctl(2) should be better equipped to deal with ENOTTY,
> > because that is an error that ioctl(2) can return in general, whereas EACCES can
> > only be returned if one of the specific subcommands returns it.
> >
> > According to the man page, ENOTTY is the error that ioctl(2) returns if the
> > given "request does not apply to the kind of object that the file descriptor fd
> > references".
> >
> > That is not 100% what is happening here, but from the errors listed in ioctl(2),
> > this seems to be the closest, semantically.
> >
> > What do you think?
>
> ENOTTY has a (kinda) well-defined semantic, which should not depend on
> an access control. Other LSMs already return EACCES for denied IOCTLs,
> so the man page should be updated with this information instead. ;)
Well, thinking about this again, for the sake of consistency with other
IOCTLs, we should not specifically handle IOCTL error codes, but instead
return either 0 or -EACCES. The network error cases are special because
the LSM is called before some user-provided data are interpreted by the
network stack, and in this case we need to interpret these data ourself.
But in the case of IOCTLs, we may only need to handle the cases where an
IOCTL command may be interpreted by different implementations (e.g. VFS
or FS implementation), but even that is not a good idea, see below.
For FIOQSIZE, I don't think anymore that we should try to mimic the
do_vfs_ioctl() implementation. In fact, this approach could end up
confusing developers and leaking metadata (see FIGETBSZ). Even with
FIONREAD, the FS (i.e. vfs_ioctl() call) should follow the same
semantic as the VFS (i.e. do_vfs_ioctl() code), so we should treat them
the same and keep it simple: either return 0 or -EACCES.
About the file_ioctl() IOCTLs, we should check that there are no
overlapping with other IOCTLs (with different name). I think we should
trust the FS implementation to implement the same semantic, but much
less the device drivers. The main issue is with VFS and FS code
returning -ENOIOCTLCMD because in this case it is forwarded to any
implementation.
However, in the case of delegation, and as a safeguard, what we could do
to avoid driver IOCTL command overlaps is to check if the file is a
block or character device. In this case, we just don't delegate any
access right but make LANDLOCK_ACCESS_FS_IOCTL handle them (we need to
manage VFS's IOCTLs that deal with block/char device though). Again, we
should make sure that -ENOIOCTLCMD will not trick us (for the delegate
cases). I'm not sure how the link between drivers and the FS storing the
related block/char device is managed though. Does that make sense?
