On 2021-05-28 11:24, Christian Brauner wrote:
> From: Christian Brauner <christian.brauner@xxxxxxxxxx>
>
> The new openat2() syscall verifies that no unknown O-flag values are
> set and returns an error to userspace if they are while the older open
> syscalls like open() and openat() simply ignore unknown flag values:
>
> #define O_FLAG_CURRENTLY_INVALID (1 << 31)
> struct open_how how = {
> .flags = O_RDONLY | O_FLAG_CURRENTLY_INVALID,
> .resolve = 0,
> };
>
> /* fails */
> fd = openat2(-EBADF, "/dev/null", &how, sizeof(how));
>
> /* succeeds */
> fd = openat(-EBADF, "/dev/null", O_RDONLY | O_FLAG_CURRENTLY_INVALID);
>
> However, openat2() silently truncates the upper 32 bits meaning:
>
> #define O_FLAG_CURRENTLY_INVALID_LOWER32 (1 << 31)
> #define O_FLAG_CURRENTLY_INVALID_UPPER32 (1 << 40)
>
> struct open_how how_lowe32 = {
> .flags = O_RDONLY | O_FLAG_CURRENTLY_INVALID_LOWER32,
> };
>
> struct open_how how_upper32 = {
> .flags = O_RDONLY | O_FLAG_CURRENTLY_INVALID_UPPER32,
> };
>
> /* fails */
> fd = openat2(-EBADF, "/dev/null", &how_lower32, sizeof(how_lower32));
>
> /* succeeds */
> fd = openat2(-EBADF, "/dev/null", &how_upper32, sizeof(how_upper32));
>
> Fix this by preventing the immediate truncation in build_open_flags().
>
> There's a snafu here though stripping FMODE_* directly from flags would
> cause the upper 32 bits to be truncated as well due to integer promotion
> rules since FMODE_* is unsigned int, O_* are signed ints (yuck).
>
> In addition, struct open_flags currently defines flags to be 32 bit
> which is reasonable. If we simply were to bump it to 64 bit we would
> need to change a lot of code preemptively which doesn't seem worth it.
> So simply add a compile-time check verifying that all currently known
> O_* flags are within the 32 bit range and fail to build if they aren't
> anymore.
>
> This change shouldn't regress old open syscalls since they silently
> truncate any unknown values anyway. It is a tiny semantic change for
> openat2() but it is very unlikely people pass ing > 32 bit unknown flags
> and the syscall is relatively new too.
>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Aleksa Sarai <cyphar@xxxxxxxxxx>
> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: linux-fsdevel@xxxxxxxxxxxxxxx
> Reported-by: Richard Guy Briggs <rgb@xxxxxxxxxx>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> Reviewed-by: Aleksa Sarai <cyphar@xxxxxxxxxx>
> Signed-off-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
Reviewed-by: Richard Guy Briggs <rgb@xxxxxxxxxx>
> ---
> /* v2 */
> - Richard Guy Briggs <rgb@xxxxxxxxxx>:
> - Add an explicit BUILD_BUG_ON() to check when we need to change
> struct open_flags to account for O_* flags > 32 bits.
> ---
> fs/open.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/fs/open.c b/fs/open.c
> index e53af13b5835..53bc0573c0ec 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -1002,12 +1002,20 @@ inline struct open_how build_open_how(int flags, umode_t mode)
>
> inline int build_open_flags(const struct open_how *how, struct open_flags *op)
> {
> - int flags = how->flags;
> + u64 flags = how->flags;
> + u64 strip = FMODE_NONOTIFY | O_CLOEXEC;
> int lookup_flags = 0;
> int acc_mode = ACC_MODE(flags);
>
> - /* Must never be set by userspace */
> - flags &= ~(FMODE_NONOTIFY | O_CLOEXEC);
> + BUILD_BUG_ON_MSG(upper_32_bits(VALID_OPEN_FLAGS),
> + "struct open_flags doesn't yet handle flags > 32 bits");
> +
> + /*
> + * Strip flags that either shouldn't be set by userspace like
> + * FMODE_NONOTIFY or that aren't relevant in determining struct
> + * open_flags like O_CLOEXEC.
> + */
> + flags &= ~strip;
>
> /*
> * Older syscalls implicitly clear all of the invalid flags or argument
> --
> 2.27.0
>
- RGB
--
Richard Guy Briggs <rgb@xxxxxxxxxx>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
