* [PATCH 1/4] Add path resolution flag LOOKUP_NEVER_FOLLOW
@ 2019-02-12 14:54 demiobenour
2019-02-12 14:54 ` demiobenour
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: demiobenour @ 2019-02-12 14:54 UTC (permalink / raw)
To: Alexander Viro, David Howells, Demi M. Obenour,
open list:FILESYSTEMS (VFS and infrastructure),
open list
From: "Demi M. Obenour" <demiobenour@gmail.com>
This adds the flag LOOKUP_NEVER_FOLLOW to path resolution, which tells
the code in fs/namei.c to never follow symlinks. This flag overrides
LOOKUP_FOLLOW, since this makes internal APIs simpler: code can set the
flag without needing to also clear LOOKUP_FOLLOW, which is often set by
default.
This is a prerequisite to adding O_PATHSTATIC, but is also useful for
kernel-internal use.
---
fs/namei.c | 4 +++-
include/linux/namei.h | 2 ++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/fs/namei.c b/fs/namei.c
index 914178cdbe94..54fbd2c7ba82 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1720,7 +1720,9 @@ static int pick_link(struct nameidata *nd, struct path *link,
{
int error;
struct saved *last;
- if (unlikely(nd->total_link_count++ >= MAXSYMLINKS)) {
+ const int max_symlinks = (nd->flags & LOOKUP_NEVER_FOLLOW) ?
+ 0 : MAXSYMLINKS;
+ if (unlikely(nd->total_link_count++ >= max_symlinks)) {
path_to_nameidata(link, nd);
return -ELOOP;
}
diff --git a/include/linux/namei.h b/include/linux/namei.h
index a78606e8e3df..f065502a653d 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -24,10 +24,12 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
* - internal "there are more path components" flag
* - dentry cache is untrusted; force a real lookup
* - suppress terminal automount
+ * - never follow symbolic links, even internally
*/
#define LOOKUP_FOLLOW 0x0001
#define LOOKUP_DIRECTORY 0x0002
#define LOOKUP_AUTOMOUNT 0x0004
+#define LOOKUP_NEVER_FOLLOW 0x0008
#define LOOKUP_PARENT 0x0010
#define LOOKUP_REVAL 0x0020
--
2.20.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/4] Expose O_PATHSTATIC to userspace
2019-02-12 14:54 [PATCH 1/4] Add path resolution flag LOOKUP_NEVER_FOLLOW demiobenour
@ 2019-02-12 14:54 ` demiobenour
2019-02-12 14:54 ` [PATCH 3/4] Add AT_PATHSTATIC to linkat() demiobenour
2019-02-12 14:54 ` [PATCH 4/4] Return -EINVAL if userspace passes bogus flags to open() demiobenour
2 siblings, 0 replies; 9+ messages in thread
From: demiobenour @ 2019-02-12 14:54 UTC (permalink / raw)
To: Jeff Layton, J. Bruce Fields, Alexander Viro, Arnd Bergmann,
open list:FILE LOCKING (flock() and fcntl()/lockf()),
open list, open list:GENERIC INCLUDE/ASM HEADER FILES
Cc: Demi M. Obenour
From: "Demi M. Obenour" <demiobenour@gmail.com>
This adds the file open flag O_PATHSTATIC, which ensures that symbolic
links are *never* followed, even in path components other than the last.
This is distinct from O_NOFOLLOW, which only prevents symlinks in the
*last* component from being followed.
This is useful for avoiding race conditions in userspace code that
should expose only a subset of the filesystem to clients. This includes
FTP and SFTP servers, QEMU, and others.
Currently, O_NOFOLLOW must be set if O_PATHSTATIC is set. Otherwise,
open() fails with -EINVAL.
---
fs/fcntl.c | 2 +-
fs/namei.c | 6 ++++++
fs/open.c | 24 ++++++++++++++++++++++--
include/linux/fcntl.h | 2 +-
include/uapi/asm-generic/fcntl.h | 4 ++++
5 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 083185174c6d..6c85c4d0c006 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1031,7 +1031,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 54fbd2c7ba82..4c90f265c103 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3282,6 +3282,12 @@ static int do_last(struct nameidata *nd,
if (!(open_flag & O_CREAT)) {
if (nd->last.name[nd->last.len])
nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
+
+ if (open_flag & O_PATHSTATIC) {
+ nd->flags |= LOOKUP_NEVER_FOLLOW;
+ nd->flags &= ~LOOKUP_FOLLOW;
+ }
+
/* we _can_ be in RCU mode here */
error = lookup_fast(nd, &path, &inode, &seq);
if (likely(error > 0))
diff --git a/fs/open.c b/fs/open.c
index 0285ce7dbd51..717afa8179c0 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -940,6 +940,24 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
/* Must never be set by userspace */
flags &= ~FMODE_NONOTIFY & ~O_CLOEXEC;
+ /*
+ * If nonzero, setting O_PATHSTATIC but not O_NOFOLLOW fails with
+ * -EINVAL. Otherwise, setting O_PATHSTATIC automatically sets
+ * O_NOFOLLOW.
+ */
+#define REQUIRE_NOFOLLOW_FOR_PATHSTATIC 1
+
+#if REQUIRE_NOFOLLOW_FOR_PATHSTATIC
+ /* O_PATHSTATIC doesn't make sense without O_NOFOLLOW */
+ if (unlikely((flags & O_PATHSTATIC) && !(flags & O_NOFOLLOW)))
+ return -EINVAL;
+#elif defined REQUIRE_NOFOLLOW_FOR_PATHSTATIC
+ if (flags & O_PATHSTATIC)
+ flags &= O_NOFOLLOW;
+#else
+#error REQUIRE_NOFOLLOW_FOR_PATHSTATIC must be defined
+#endif
+
/*
* O_SYNC is implemented as __O_SYNC|O_DSYNC. As many places only
* check for O_DSYNC if the need any syncing at all we enforce it's
@@ -959,7 +977,7 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
* If we have O_PATH in the open flag. Then we
* cannot have anything other than the below set of flags
*/
- flags &= O_DIRECTORY | O_NOFOLLOW | O_PATH;
+ flags &= O_DIRECTORY | O_NOFOLLOW | O_PATH | O_PATHSTATIC;
acc_mode = 0;
}
@@ -986,7 +1004,9 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
if (flags & O_DIRECTORY)
lookup_flags |= LOOKUP_DIRECTORY;
- if (!(flags & O_NOFOLLOW))
+ if (flags & O_PATHSTATIC)
+ lookup_flags |= LOOKUP_NEVER_FOLLOW;
+ else if (!(flags & O_NOFOLLOW))
lookup_flags |= LOOKUP_FOLLOW;
op->lookup_flags = lookup_flags;
return 0;
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 27dc7a60693e..6f91e1490592 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_PATHSTATIC)
#ifndef force_o_largefile
#define force_o_largefile() (BITS_PER_LONG != 32)
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 9dc0bf0c5a6e..314ea1cecf44 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -89,6 +89,10 @@
#define __O_TMPFILE 020000000
#endif
+#ifndef O_PATHSTATIC
+#define O_PATHSTATIC 040000000
+#endif
+
/* a horrid kludge trying to make sure that this will fail on old kernels */
#define O_TMPFILE (__O_TMPFILE | O_DIRECTORY)
#define O_TMPFILE_MASK (__O_TMPFILE | O_DIRECTORY | O_CREAT)
--
2.20.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/4] Expose O_PATHSTATIC to userspace
@ 2019-02-12 14:54 ` demiobenour
0 siblings, 0 replies; 9+ messages in thread
From: demiobenour @ 2019-02-12 14:54 UTC (permalink / raw)
To: Jeff Layton, J. Bruce Fields, Alexander Viro, Arnd Bergmann,
open list:FILE LOCKING flock and fcntl/lockf, open list,
open list:GENERIC INCLUDE/ASM HEADER FILES
Cc: Demi M. Obenour
From: "Demi M. Obenour" <demiobenour@gmail.com>
This adds the file open flag O_PATHSTATIC, which ensures that symbolic
links are *never* followed, even in path components other than the last.
This is distinct from O_NOFOLLOW, which only prevents symlinks in the
*last* component from being followed.
This is useful for avoiding race conditions in userspace code that
should expose only a subset of the filesystem to clients. This includes
FTP and SFTP servers, QEMU, and others.
Currently, O_NOFOLLOW must be set if O_PATHSTATIC is set. Otherwise,
open() fails with -EINVAL.
---
fs/fcntl.c | 2 +-
fs/namei.c | 6 ++++++
fs/open.c | 24 ++++++++++++++++++++++--
include/linux/fcntl.h | 2 +-
include/uapi/asm-generic/fcntl.h | 4 ++++
5 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 083185174c6d..6c85c4d0c006 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1031,7 +1031,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 54fbd2c7ba82..4c90f265c103 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3282,6 +3282,12 @@ static int do_last(struct nameidata *nd,
if (!(open_flag & O_CREAT)) {
if (nd->last.name[nd->last.len])
nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
+
+ if (open_flag & O_PATHSTATIC) {
+ nd->flags |= LOOKUP_NEVER_FOLLOW;
+ nd->flags &= ~LOOKUP_FOLLOW;
+ }
+
/* we _can_ be in RCU mode here */
error = lookup_fast(nd, &path, &inode, &seq);
if (likely(error > 0))
diff --git a/fs/open.c b/fs/open.c
index 0285ce7dbd51..717afa8179c0 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -940,6 +940,24 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
/* Must never be set by userspace */
flags &= ~FMODE_NONOTIFY & ~O_CLOEXEC;
+ /*
+ * If nonzero, setting O_PATHSTATIC but not O_NOFOLLOW fails with
+ * -EINVAL. Otherwise, setting O_PATHSTATIC automatically sets
+ * O_NOFOLLOW.
+ */
+#define REQUIRE_NOFOLLOW_FOR_PATHSTATIC 1
+
+#if REQUIRE_NOFOLLOW_FOR_PATHSTATIC
+ /* O_PATHSTATIC doesn't make sense without O_NOFOLLOW */
+ if (unlikely((flags & O_PATHSTATIC) && !(flags & O_NOFOLLOW)))
+ return -EINVAL;
+#elif defined REQUIRE_NOFOLLOW_FOR_PATHSTATIC
+ if (flags & O_PATHSTATIC)
+ flags &= O_NOFOLLOW;
+#else
+#error REQUIRE_NOFOLLOW_FOR_PATHSTATIC must be defined
+#endif
+
/*
* O_SYNC is implemented as __O_SYNC|O_DSYNC. As many places only
* check for O_DSYNC if the need any syncing at all we enforce it's
@@ -959,7 +977,7 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
* If we have O_PATH in the open flag. Then we
* cannot have anything other than the below set of flags
*/
- flags &= O_DIRECTORY | O_NOFOLLOW | O_PATH;
+ flags &= O_DIRECTORY | O_NOFOLLOW | O_PATH | O_PATHSTATIC;
acc_mode = 0;
}
@@ -986,7 +1004,9 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
if (flags & O_DIRECTORY)
lookup_flags |= LOOKUP_DIRECTORY;
- if (!(flags & O_NOFOLLOW))
+ if (flags & O_PATHSTATIC)
+ lookup_flags |= LOOKUP_NEVER_FOLLOW;
+ else if (!(flags & O_NOFOLLOW))
lookup_flags |= LOOKUP_FOLLOW;
op->lookup_flags = lookup_flags;
return 0;
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 27dc7a60693e..6f91e1490592 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_PATHSTATIC)
#ifndef force_o_largefile
#define force_o_largefile() (BITS_PER_LONG != 32)
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 9dc0bf0c5a6e..314ea1cecf44 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -89,6 +89,10 @@
#define __O_TMPFILE 020000000
#endif
+#ifndef O_PATHSTATIC
+#define O_PATHSTATIC 040000000
+#endif
+
/* a horrid kludge trying to make sure that this will fail on old kernels */
#define O_TMPFILE (__O_TMPFILE | O_DIRECTORY)
#define O_TMPFILE_MASK (__O_TMPFILE | O_DIRECTORY | O_CREAT)
--
2.20.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/4] Add AT_PATHSTATIC to linkat()
2019-02-12 14:54 [PATCH 1/4] Add path resolution flag LOOKUP_NEVER_FOLLOW demiobenour
2019-02-12 14:54 ` demiobenour
@ 2019-02-12 14:54 ` demiobenour
2019-02-12 14:54 ` [PATCH 4/4] Return -EINVAL if userspace passes bogus flags to open() demiobenour
2 siblings, 0 replies; 9+ messages in thread
From: demiobenour @ 2019-02-12 14:54 UTC (permalink / raw)
To: Alexander Viro, Jeff Layton, J. Bruce Fields,
open list:FILESYSTEMS (VFS and infrastructure),
open list
Cc: Demi M. Obenour
From: "Demi M. Obenour" <demiobenour@gmail.com>
This has the same meaning as O_PATHSTATIC does in openat(), and has the
same uses.
---
fs/namei.c | 8 +++++++-
include/uapi/linux/fcntl.h | 1 +
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/fs/namei.c b/fs/namei.c
index 4c90f265c103..b47f89af00f2 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4291,8 +4291,14 @@ int do_linkat(int olddfd, const char __user *oldname, int newdfd,
how = LOOKUP_EMPTY;
}
- if (flags & AT_SYMLINK_FOLLOW)
+ if (flags & AT_SYMLINK_FOLLOW) {
+ if (flags & AT_PATHSTATIC)
+ return -EINVAL;
how |= LOOKUP_FOLLOW;
+ }
+
+ if (flags & AT_PATHSTATIC)
+ how |= LOOKUP_NEVER_FOLLOW;
retry:
error = user_path_at(olddfd, oldname, how, &old_path);
if (error)
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 6448cdd9a350..a2f65635c8fc 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -89,6 +89,7 @@
#define AT_STATX_SYNC_AS_STAT 0x0000 /* - Do whatever stat() does */
#define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */
#define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */
+#define AT_PATHSTATIC 0x8000 /* Do not follow symbolic links anywhere. */
#endif /* _UAPI_LINUX_FCNTL_H */
--
2.20.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 4/4] Return -EINVAL if userspace passes bogus flags to open()
2019-02-12 14:54 [PATCH 1/4] Add path resolution flag LOOKUP_NEVER_FOLLOW demiobenour
2019-02-12 14:54 ` demiobenour
2019-02-12 14:54 ` [PATCH 3/4] Add AT_PATHSTATIC to linkat() demiobenour
@ 2019-02-12 14:54 ` demiobenour
2019-02-12 20:38 ` Al Viro
2 siblings, 1 reply; 9+ messages in thread
From: demiobenour @ 2019-02-12 14:54 UTC (permalink / raw)
To: Alexander Viro, open list:FILESYSTEMS (VFS and infrastructure),
open list
Cc: Demi M. Obenour
From: "Demi M. Obenour" <demiobenour@gmail.com>
While testing the O_PATHSTATIC patch, I discovered that Linux does not
return any error if an invalid flag is passed to open(2). This prevents
adding new flags without a (minor) risk of breaking userspace.
Therefore, add a check for invalid flags, and return -EINVAL if any are
found.
---
fs/open.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/fs/open.c b/fs/open.c
index 717afa8179c0..eeaa2eeb342a 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1074,6 +1074,13 @@ long do_sys_open(int dfd, const char __user *filename, int flags, umode_t mode)
if (fd)
return fd;
+ /*
+ * Enforce that open flags are valid, to ensure that new flags can be
+ * added later.
+ */
+ if (unlikely(flags & ~VALID_OPEN_FLAGS))
+ return -EINVAL;
+
tmp = getname(filename);
if (IS_ERR(tmp))
return PTR_ERR(tmp);
--
2.20.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] Expose O_PATHSTATIC to userspace
2019-02-12 14:54 ` demiobenour
(?)
@ 2019-02-12 20:18 ` Andreas Dilger
2019-02-12 22:41 ` Demi M. Obenour
-1 siblings, 1 reply; 9+ messages in thread
From: Andreas Dilger @ 2019-02-12 20:18 UTC (permalink / raw)
To: demiobenour
Cc: Jeff Layton, J. Bruce Fields, Alexander Viro, Arnd Bergmann,
linux-fsdevel, open list, linux-arch
[-- Attachment #1: Type: text/plain, Size: 986 bytes --]
On Feb 12, 2019, at 7:54 AM, demiobenour@gmail.com wrote:
>
> From: "Demi M. Obenour" <demiobenour@gmail.com>
>
> This adds the file open flag O_PATHSTATIC, which ensures that symbolic
> links are *never* followed, even in path components other than the last.
> This is distinct from O_NOFOLLOW, which only prevents symlinks in the
> *last* component from being followed.
>
> This is useful for avoiding race conditions in userspace code that
> should expose only a subset of the filesystem to clients. This includes
> FTP and SFTP servers, QEMU, and others.
>
> Currently, O_NOFOLLOW must be set if O_PATHSTATIC is set. Otherwise,
> open() fails with -EINVAL.
I don't want to bikeshed (discard suggestion if you disagree), but why not
name the flag "O_NEVER_FOLLOW" so that users can see it is also related to
"O_NOFOLLOW"? Otherwise it seems like they are two completely different
things from looking at the names, when in fact they are closely related.
Cheers, Andreas
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 4/4] Return -EINVAL if userspace passes bogus flags to open()
2019-02-12 14:54 ` [PATCH 4/4] Return -EINVAL if userspace passes bogus flags to open() demiobenour
@ 2019-02-12 20:38 ` Al Viro
2019-02-12 23:00 ` Demi M. Obenour
0 siblings, 1 reply; 9+ messages in thread
From: Al Viro @ 2019-02-12 20:38 UTC (permalink / raw)
To: demiobenour; +Cc: open list:FILESYSTEMS (VFS and infrastructure), open list
On Tue, Feb 12, 2019 at 09:54:47AM -0500, demiobenour@gmail.com wrote:
> From: "Demi M. Obenour" <demiobenour@gmail.com>
>
> While testing the O_PATHSTATIC patch, I discovered that Linux does not
> return any error if an invalid flag is passed to open(2). This prevents
> adding new flags without a (minor) risk of breaking userspace.
> Therefore, add a check for invalid flags, and return -EINVAL if any are
> found.
... which would qualtify as userland ABI breakage all by itself.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] Expose O_PATHSTATIC to userspace
2019-02-12 20:18 ` Andreas Dilger
@ 2019-02-12 22:41 ` Demi M. Obenour
0 siblings, 0 replies; 9+ messages in thread
From: Demi M. Obenour @ 2019-02-12 22:41 UTC (permalink / raw)
To: Andreas Dilger
Cc: Jeff Layton, J. Bruce Fields, Alexander Viro, Arnd Bergmann,
linux-fsdevel, open list, linux-arch
[-- Attachment #1.1: Type: text/plain, Size: 1425 bytes --]
On 2/12/19 3:18 PM, Andreas Dilger wrote:
> On Feb 12, 2019, at 7:54 AM, demiobenour@gmail.com wrote:
>>
>> From: "Demi M. Obenour" <demiobenour@gmail.com>
>>
>> This adds the file open flag O_PATHSTATIC, which ensures that symbolic
>> links are *never* followed, even in path components other than the last.
>> This is distinct from O_NOFOLLOW, which only prevents symlinks in the
>> *last* component from being followed.
>>
>> This is useful for avoiding race conditions in userspace code that
>> should expose only a subset of the filesystem to clients. This includes
>> FTP and SFTP servers, QEMU, and others.
>>
>> Currently, O_NOFOLLOW must be set if O_PATHSTATIC is set. Otherwise,
>> open() fails with -EINVAL.
>
> I don't want to bikeshed (discard suggestion if you disagree), but why not
> name the flag "O_NEVER_FOLLOW" so that users can see it is also related to
> "O_NOFOLLOW"? Otherwise it seems like they are two completely different
> things from looking at the names, when in fact they are closely related.
>
> Cheers, Andreas
>
Searching for O_PATHSTATIC gives two results:
* https://www.halfdog.net/Security/2010/FilesystemRecursionAndSymlinks
* https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg06225.html
O_DIRECTORY_NOFOLLOW would also be a good choice, since that is what the
flag actually does. Ideally, we would rename O_NOFOLLOW, but we can’t.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 4/4] Return -EINVAL if userspace passes bogus flags to open()
2019-02-12 20:38 ` Al Viro
@ 2019-02-12 23:00 ` Demi M. Obenour
0 siblings, 0 replies; 9+ messages in thread
From: Demi M. Obenour @ 2019-02-12 23:00 UTC (permalink / raw)
To: Al Viro; +Cc: open list:FILESYSTEMS (VFS and infrastructure), open list
[-- Attachment #1.1: Type: text/plain, Size: 843 bytes --]
On 2/12/19 3:38 PM, Al Viro wrote:
> On Tue, Feb 12, 2019 at 09:54:47AM -0500, demiobenour@gmail.com wrote:
>> From: "Demi M. Obenour" <demiobenour@gmail.com>
>>
>> While testing the O_PATHSTATIC patch, I discovered that Linux does not
>> return any error if an invalid flag is passed to open(2). This prevents
>> adding new flags without a (minor) risk of breaking userspace.
>> Therefore, add a check for invalid flags, and return -EINVAL if any are
>> found.
>
> ... which would qualtify as userland ABI breakage all by itself.
>
I suspect that very few (if any) programs pass invalid flags to open().
Additionally, O_DIRECT was added in Linux 2.4, even though it had
previously been ignored. If someone knows of a real program that does,
I can instead create an open2() syscall, but I would prefer to avoid that.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-02-12 23:00 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-12 14:54 [PATCH 1/4] Add path resolution flag LOOKUP_NEVER_FOLLOW demiobenour
2019-02-12 14:54 ` [PATCH 2/4] Expose O_PATHSTATIC to userspace demiobenour
2019-02-12 14:54 ` demiobenour
2019-02-12 20:18 ` Andreas Dilger
2019-02-12 22:41 ` Demi M. Obenour
2019-02-12 14:54 ` [PATCH 3/4] Add AT_PATHSTATIC to linkat() demiobenour
2019-02-12 14:54 ` [PATCH 4/4] Return -EINVAL if userspace passes bogus flags to open() demiobenour
2019-02-12 20:38 ` Al Viro
2019-02-12 23:00 ` Demi M. Obenour
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.