On 2019-10-19, Aleksa Sarai wrote: > On 2019-10-15, Omar Sandoval wrote: > > From: Omar Sandoval > > > > The upcoming RWF_ENCODED operation introduces some security concerns: > > > > 1. Compressed writes will pass arbitrary data to decompression > > algorithms in the kernel. > > 2. Compressed reads can leak truncated/hole punched data. > > > > Therefore, we need to require privilege for RWF_ENCODED. It's not > > possible to do the permissions checks at the time of the read or write > > because, e.g., io_uring submits IO from a worker thread. So, add an open > > flag which requires CAP_SYS_ADMIN. It can also be set and cleared with > > fcntl(). The flag is not cleared in any way on fork or exec; it should > > probably be used with O_CLOEXEC in most cases. > > > > Note that the usual issue that unknown open flags are ignored doesn't > > really matter for O_ENCODED; if the kernel doesn't support O_ENCODED, > > then it doesn't support RWF_ENCODED, either. I also disagree with this statement -- if an old userspace program sets O_ENCODED it will now get an -EPERM if it doesn't have CAP_SYS_ADMIN. That is a break in backwards compatibility. > > > > Signed-off-by: Omar Sandoval > > --- > > fs/fcntl.c | 10 ++++++++-- > > fs/namei.c | 4 ++++ > > include/linux/fcntl.h | 2 +- > > include/uapi/asm-generic/fcntl.h | 4 ++++ > > 4 files changed, 17 insertions(+), 3 deletions(-) > > > > diff --git a/fs/fcntl.c b/fs/fcntl.c > > index 3d40771e8e7c..45ebc6df078e 100644 > > --- a/fs/fcntl.c > > +++ b/fs/fcntl.c > > @@ -30,7 +30,8 @@ > > #include > > #include > > > > -#define SETFL_MASK (O_APPEND | O_NONBLOCK | O_NDELAY | O_DIRECT | O_NOATIME) > > +#define SETFL_MASK (O_APPEND | O_NONBLOCK | O_NDELAY | O_DIRECT | O_NOATIME | \ > > + O_ENCODED) > > > > static int setfl(int fd, struct file * filp, unsigned long arg) > > { > > @@ -49,6 +50,11 @@ static int setfl(int fd, struct file * filp, unsigned long arg) > > if (!inode_owner_or_capable(inode)) > > return -EPERM; > > > > + /* O_ENCODED can only be set by superuser */ > > + if ((arg & O_ENCODED) && !(filp->f_flags & O_ENCODED) && > > + !capable(CAP_SYS_ADMIN)) > > + return -EPERM; > > I have a feeling the error should probably be an EACCES and not EPERM. > > > + > > /* required for strict SunOS emulation */ > > if (O_NONBLOCK != O_NDELAY) > > if (arg & O_NDELAY) > > @@ -1031,7 +1037,7 @@ static int __init fcntl_init(void) > > * Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY > > * is defined as O_NONBLOCK on some platforms and not on others. > > */ > > - BUILD_BUG_ON(21 - 1 /* for O_RDONLY being 0 */ != > > + BUILD_BUG_ON(22 - 1 /* for O_RDONLY being 0 */ != > > HWEIGHT32( > > (VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) | > > __FMODE_EXEC | __FMODE_NONOTIFY)); > > diff --git a/fs/namei.c b/fs/namei.c > > index 671c3c1a3425..ae86b125888a 100644 > > --- a/fs/namei.c > > +++ b/fs/namei.c > > @@ -2978,6 +2978,10 @@ static int may_open(const struct path *path, int acc_mode, int flag) > > if (flag & O_NOATIME && !inode_owner_or_capable(inode)) > > return -EPERM; > > > > + /* O_ENCODED can only be set by superuser */ > > + if ((flag & O_ENCODED) && !capable(CAP_SYS_ADMIN)) > > + return -EPERM; > > I would suggest that this check be put into build_open_flags() rather > than putting it this late in open(). Also, same nit about the error > return as above. > > > + > > return 0; > > } > > > > diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h > > index d019df946cb2..5fac02479639 100644 > > --- a/include/linux/fcntl.h > > +++ b/include/linux/fcntl.h > > @@ -9,7 +9,7 @@ > > (O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC | \ > > O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \ > > FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \ > > - O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE) > > + O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_ENCODED) > > > > #ifndef force_o_largefile > > #define force_o_largefile() (!IS_ENABLED(CONFIG_ARCH_32BIT_OFF_T)) > > diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h > > index 9dc0bf0c5a6e..8c5cbd5942e3 100644 > > --- a/include/uapi/asm-generic/fcntl.h > > +++ b/include/uapi/asm-generic/fcntl.h > > @@ -97,6 +97,10 @@ > > #define O_NDELAY O_NONBLOCK > > #endif > > > > +#ifndef O_ENCODED > > +#define O_ENCODED 040000000 > > +#endif > > You should also define this for all of the architectures which don't use > the generic O_* flag values. On alpha, O_PATH is equal to the value you > picked (just be careful on sparc -- 0x4000000 is the next free bit, but > it's used by FMODE_NONOTIFY.) > > > + > > #define F_DUPFD 0 /* dup */ > > #define F_GETFD 1 /* get close_on_exec */ > > #define F_SETFD 2 /* set/clear close_on_exec */ > > -- > Aleksa Sarai > Senior Software Engineer (Containers) > SUSE Linux GmbH > -- Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH