Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixup largefile define #31419

Merged
merged 2 commits into from
Feb 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/basic/fd-util.h
Expand Up @@ -8,6 +8,7 @@
#include <sys/socket.h>

#include "macro.h"
#include "missing_fcntl.h"
#include "stdio-util.h"

/* maximum length of fdname */
Expand All @@ -21,6 +22,20 @@
#define EBADF_PAIR { -EBADF, -EBADF }
#define EBADF_TRIPLET { -EBADF, -EBADF, -EBADF }

/* Flags that are safe to have set on an FD given to a privileged service to operate on.
* This ensures that clients can't trick a privileged service into giving access to a file the client
* doesn't already have access to (especially via something like O_PATH).
*
* O_NOFOLLOW: For some reason the kernel will return this flag from fcntl; it doesn't go away immediately
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, this is new w/ this PR. Just double checking that this was noticed

* after open(). It should have no effect whatsoever to an already-opened FD, but if it does
* it's decreasing the risk to a privileged service since it disables symlink following.
*
* RAW_O_LARGEFILE: glibc secretly sets this and neglects to hide it from us if we call fcntl. See comment
* in missing_fcntl.h for more details about this.
*/
#define SAFE_FD_FLAGS (O_ACCMODE|O_NOFOLLOW|RAW_O_LARGEFILE)
#define UNSAFE_FD_FLAGS(flags) ((unsigned)(flags) & ~SAFE_FD_FLAGS)

int close_nointr(int fd);
int safe_close(int fd);
void safe_close_pair(int p[static 2]);
Expand Down
21 changes: 19 additions & 2 deletions src/basic/missing_fcntl.h
Expand Up @@ -69,9 +69,26 @@

/* So O_LARGEFILE is generally implied by glibc, and defined to zero hence, because we only build in LFS
* mode. However, when invoking fcntl(F_GETFL) the flag is ORed into the result anyway — glibc does not mask
* it away. Which sucks. Let's define the actual value here, so that we can mask it ourselves. */
* it away. Which sucks. Let's define the actual value here, so that we can mask it ourselves.
*
* The precise definition is arch specific, so we use the values defined in the kernel (note that some
* are hexa and others are octal; duplicated as-is from the kernel definitions):
* - alpha, arm, arm64, m68k, mips, parisc, powerpc, sparc: each has a specific value;
* - others: they use the "generic" value (defined in include/uapi/asm-generic/fcntl.h) */
#if O_LARGEFILE != 0
#define RAW_O_LARGEFILE O_LARGEFILE
#else
#define RAW_O_LARGEFILE 0100000
#if defined(__alpha__) || defined(__arm__) || defined(__aarch64__) || defined(__m68k__)
#define RAW_O_LARGEFILE 0400000
#elif defined(__mips__)
#define RAW_O_LARGEFILE 0x2000
#elif defined(__parisc__) || defined(__hppa__)
#define RAW_O_LARGEFILE 000004000
#elif defined(__powerpc__)
#define RAW_O_LARGEFILE 0200000
#elif defined(__sparc__)
#define RAW_O_LARGEFILE 0x40000
#else
#define RAW_O_LARGEFILE 00100000
#endif
#endif
7 changes: 4 additions & 3 deletions src/home/homed-bus.c
Expand Up @@ -3,7 +3,6 @@
#include "fd-util.h"
#include "home-util.h"
#include "homed-bus.h"
#include "missing_fcntl.h"
#include "stat-util.h"
#include "strv.h"

Expand Down Expand Up @@ -120,8 +119,10 @@ int bus_message_read_blobs(sd_bus_message *m, Hashmap **ret, sd_bus_error *error

/* Refuse fds w/ unexpected flags set. In particular, we don't want to permit O_PATH FDs, since
* those don't actually guarantee that the client has access to the file. */
if ((flags & ~(O_ACCMODE|RAW_O_LARGEFILE)) != 0)
return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "FD for %s has unexpected flags set", filename);
if (UNSAFE_FD_FLAGS(flags) != 0)
return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS,
"FD for %s has unexpected flags set: 0%o",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think this is probably too verbose to be included in bus error. We should add a helper that does log_debug internally. But I'll merge this as-is and submit a follow-up.

filename, UNSAFE_FD_FLAGS(flags));

r = hashmap_put(blobs, filename, FD_TO_PTR(fd));
if (r < 0)
Expand Down
7 changes: 4 additions & 3 deletions src/journal/journald-native.c
Expand Up @@ -22,7 +22,6 @@
#include "journald-wall.h"
#include "memfd-util.h"
#include "memory-util.h"
#include "missing_fcntl.h"
#include "parse-util.h"
#include "path-util.h"
#include "process-util.h"
Expand Down Expand Up @@ -363,8 +362,10 @@ void server_process_native_file(
return;
}

if ((flags & ~(O_ACCMODE|RAW_O_LARGEFILE)) != 0) {
log_ratelimit_error(JOURNAL_LOG_RATELIMIT, "Unexpected flags of passed memory fd, ignoring message: %m");
if (UNSAFE_FD_FLAGS(flags) != 0) {
log_ratelimit_error(JOURNAL_LOG_RATELIMIT,
"Unexpected flags of passed memory fd (0%o), ignoring message: %m",
UNSAFE_FD_FLAGS(flags));
return;
}

Expand Down