* Re: [PATCH v2 0/2] implement OA2_INHERIT_CRED flag for openat2()
From: stsp @ 2024-04-23 22:52 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <4D2A1543-273F-417F-921B-E9F994FBF2E8@amacapital.net>
23.04.2024 19:44, Andy Lutomirski пишет:
> Also, there are lots of ways that f_cred could be relevant: fsuid/fsgid, effective capabilities and security labels. And it gets more complex if this ever gets extended to support connecting or sending to a socket or if someone opens a device node. Does CAP_SYS_ADMIN carry over?
I posted a v3 where I only override
fsuid, fsgid and group_info.
Capabilities and whatever else are
not overridden to avoid security risks.
Does this address your concern?
Note that I think your other concerns
are already addressed, I just added a
bit more of a description now.
^ permalink raw reply
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Greg KH @ 2024-04-23 23:24 UTC (permalink / raw)
To: Sreenivas Bagalkote
Cc: Jonathan Cameron, Dan Williams, linux-cxl, Brett Henning,
Harold Johnson, Sumanesh Samanta, linux-kernel, Davidlohr Bueso,
Dave Jiang, Alison Schofield, Vishal Verma, Ira Weiny, linuxarm,
linux-api, Lorenzo Pieralisi, Natu, Mahesh
In-Reply-To: <CACX_a4UM7wqb_eGSP2m2f2ytQGB3j+3Y4iP2H1UfMdVmm2a+=w@mail.gmail.com>
On Tue, Apr 23, 2024 at 04:44:37PM -0600, Sreenivas Bagalkote wrote:
> This electronic communication and the information and any files transmitted
> with it, or attached to it, are confidential and are intended solely for
> the use of the individual or entity to whom it is addressed and may contain
> information that is confidential, legally privileged, protected by privacy
> laws, or otherwise restricted from disclosure to anyone else. If you are
> not the intended recipient or the person responsible for delivering the
> e-mail to the intended recipient, you are hereby notified that any use,
> copying, distributing, dissemination, forwarding, printing, or copying of
> this e-mail is strictly prohibited. If you received this e-mail in error,
> please return the e-mail to the sender, delete it from your computer, and
> destroy any printed copy of it.
Now deleted.
^ permalink raw reply
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Dan Williams @ 2024-04-24 0:07 UTC (permalink / raw)
To: Jonathan Cameron, Dan Williams
Cc: linux-cxl, Sreenivas Bagalkote, Brett Henning, Harold Johnson,
Sumanesh Samanta, linux-kernel, Davidlohr Bueso, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, linuxarm, linux-api,
Lorenzo Pieralisi, Natu, Mahesh
In-Reply-To: <20240410124517.000075f2@Huawei.com>
Jonathan Cameron wrote:
[..]
> > It is not clear to me that this material makes sense to house in
> > drivers/ vs tools/ or even out-of-tree just for maintenance burden
> > relief of keeping the universes separated. What does the Linux kernel
> > project get out of carrying this in mainline alongside the inband code?
>
> I'm not sure what you mean by in band. Aim here was to discuss
> in-band drivers for switch CCI etc. Same reason from a kernel point of
> view for why we include embedded drivers. I'll interpret in band
> as host driven and not inband as FM-API stuff.
>
> > I do think the mailbox refactoring to support non-CXL use cases is
> > interesting, but only so far as refactoring is consumed for inband use
> > cases like RAS API.
>
> If I read this right, I disagree with the 'only so far' bit.
>
> In all substantial ways we should support BMC use case of the Linux Kernel
> at a similar level to how we support forms of Linux Distros.
I think we need to talk in terms of specifics, because in the general
case I do not see the blockage. OpenBMC currently is based on v6.6.28
and carries 136 patches. An additional patch to turn off raw commands
restrictions over there would not even be noticed.
> It may not be our target market as developers for particular parts of
> our companies, but we should not block those who want to support it.
It is also the case that there is a responsibility to build maintainable
kernel interfaces that can be reasoned about, especially with devices as
powerful as CXL that are trusted to host system memory and be caching
agents. For example, I do not want to be in the position of auditing
whether proposed tunnels and passthroughs violate lockdown expectations.
Also, the assertion that these kernels will be built with
CONFIG_SECURITY_LOCKDOWN_LSM=n and likely CONFIG_STRICT_DEVMEM=n, then
the entire user-mode driver ABI is available for use. CXL commands are
simple polled mmio, does Linux really benefit from carrying drivers in
the kernel that the kernel itself does not care about?
[..]
> Switch CCI Driver: PCI driver doing everything beyond the CXL mbox specific bit.
> Type 3 Stack: All the normal stack just with the CXL Mailbox specific stuff factored
> out. Note we can move different amounts of shared logic in here, but
> in essence it deals with the extra layer on top of the raw MMPT mbox.
> MMPT Mbox: Mailbox as per the PCI spec.
> RAS API: Shared RAS API specific infrastructure used by other drivers.
Once the CXL mailbox core is turned into a library for kernel internal
consumers, like RAS API, or CXL accelerators, then it becomes easier to
add a Switch CCI consumer (perhaps as an out-of-tree module in tools/),
but it is still not clear why the kernel benefits from that arrangement.
This is less about blocking developers that have different goals it is
about finding the right projects / places to solve the problem
especially when disjoint design goals are in play and user space drivers
might be in reach.
[..]
> > > The various CXL upstream developers and maintainers may have
> > > differing views of course, but my current understanding is we want
> > > to support 1 and 2, but are very resistant to 3!
> >
> > 1, yes, 2, need to see the patches, and agree on 3.
>
> If we end up with top architecture of the diagrams above, 2 will look pretty
> similar to last version of the switch-cci patches. So raw commands only + taint.
> Factoring out MMPT is another layer that doesn't make that much difference in
> practice to this discussion. Good to have, but the reuse here would be one layer
> above that.
>
> Or we just say go for second proposed architecture and 0 impact on the
> CXL specific code, just reuse of the MMPT layer. I'd imagine people will get
> grumpy on code duplication (and we'll spend years rejecting patch sets that
> try to share the cdoe) but there should be no maintenance burden as
> a result.
I am assuming that the shared code between MMPT and CXL will happen and
that all of the command infrastructure is where centralized policy can
not keep up. If OpenBMC wants to land a driver that consumes the MMPT
core in tools/ that would seem to satisfy both the concerns of mainline
not shipping ABI that host kernels need to strictly reason about while
letting OpenBMC not need to carry out-of-tree patches indefinitely.
^ permalink raw reply
* Re: [PATCH v2] usb: gadget: f_uac2: Expose all string descriptors through configfs.
From: Pavel Hofman @ 2024-04-24 7:55 UTC (permalink / raw)
To: Chris Wulff, linux-usb@vger.kernel.org
Cc: Greg Kroah-Hartman, James Gruber, Lee Jones,
linux-kernel@vger.kernel.org, linux-api@vger.kernel.org
In-Reply-To: <CO1PR17MB541989646698286B2B13CF23E1112@CO1PR17MB5419.namprd17.prod.outlook.com>
On 23. 04. 24 19:22, Chris Wulff wrote:
>> From: Pavel Hofman <pavel.hofman@ivitera.com>
>> Sent: Tuesday, April 23, 2024 11:38 AM
>
>>> + p_it_name playback input terminal name
>>> + p_ot_name playback output terminal name
>>> + p_fu_name playback function unit name
>>> + p_alt0_name playback alt mode 0 name
>>> + p_alt1_name playback alt mode 1 name
>>
>> Nacked-by: Pavel Hofman <pavel.hofman@ivitera.com>
>>
>> I am not sure adding a numbered parameter for every additional alt mode
>> is a way to go for the future. I am not that much concerned about UAC1,
>> but IMO (at least) in UAC2 the configuration method should be flexible
>> for more alt setttings. I can see use cases with many more altsettings.
>>
>> My proposal for adding more alt settings
>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/35be4668-58d3-894a-72cf-de1afaacae45@ivitera.com/__;!!HBnMciuwfVSXJQ!TYg7j7-fh3eZAzPfiONi2lo54mf2qsWtpG0nwdaQwSqd1nGdKkTDN8o6_lSIWlWPtHoc-2Nz1KCbRhiXJnzXO8Ku1w$
>> suggested using lists to existing parameters where each item would
>> correspond to the alt setting of the same index (+1). That would allow
>> using more altsettings easily, without having to add parameters to the
>> source code and adding configfs params. I received no feedback. I do not
>> push the param list proposal, but I am convinced an acceptable solution
>> should be discussed thoroughly by the UAC2 gadget stakeholders.
>>
>> I am afraid that once p_alt1_name/c_alt1_name params are accepted, there
>> will be no way back because subsequent removal of configfs params could
>> be viewed as a regression for users.
>
> I have been thinking about this as well. The alt names are slightly different than the rest of the settings
> since they also include alt mode 0. I was thinking p/c_alt1_name could be expanded to the array so
> that the entries line up with the other settings and don't have an extra entry for alt 0. Perhaps a different
> name would make more sense.
>
> Along those lines, I didn't see any gadget drivers using an array of strings for anything, which is also why
> I didn't try to do anything here that merged alt0/1 names into an array. If we were to do an array of strings
> I'm not sure what the best separator would be. Maybe ";"? The rates array uses ",".
>
> This patch only exposes the existing strings to make them configurable, but I don't want to do anything
> that would preclude a nice interface for extra alt modes.
>
Thanks a lot for your response. Please can you take a look at
https://lore.kernel.org/linux-usb/72e9b581-4a91-2319-cb9f-0bcb370f34a1@ivitera.com/T/#m68560853b0c7bc2478942d1f953caa2ac95512bd
?
If the params in the upper level were to stand as defaults for the
altsettings (and for the existing altsetting 1 if no specific altset
subdir configs were given), maybe the naming xxx_alt1_xxx could become a
bit confusing. E.g. p_altx_name or p_alt_non0_name?
Thanks a lot,
Pavel.
^ permalink raw reply
* [PATCH v4 0/2] implement OA2_INHERIT_CRED flag for openat2()
From: Stas Sergeev @ 2024-04-24 10:52 UTC (permalink / raw)
To: linux-kernel
Cc: Stas Sergeev, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche
This patch-set implements the OA2_INHERIT_CRED flag for openat2() syscall.
It is needed to perform an open operation with the creds that were in
effect when the dir_fd was opened. This allows the process to pre-open
some dirs and switch eUID (and other UIDs/GIDs) to the less-privileged
user, while still retaining the possibility to open/create files within
the pre-opened directory set.
The sand-boxing is security-oriented: symlinks leading outside of a
sand-box are rejected. /proc magic links are rejected.
The more detailed description (including security considerations)
is available in the log messages of individual patches.
Changes in v4:
- add optimizations suggested by David Laight <David.Laight@ACULAB.COM>
- move security checks to build_open_flags()
- force RESOLVE_NO_MAGICLINKS as suggested by Andy Lutomirski <luto@kernel.org>
Changes in v3:
- partially revert v2 changes to avoid overriding capabilities.
Only the bare minimum is overridden: fsuid, fsgid and group_info.
Document the fact the full cred override is unwanted, as it may
represent an unneeded security risk.
Changes in v2:
- capture full struct cred instead of just fsuid/fsgid.
Suggested by Stefan Metzmacher <metze@samba.org>
CC: Stefan Metzmacher <metze@samba.org>
CC: Eric Biederman <ebiederm@xmission.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Andy Lutomirski <luto@kernel.org>
CC: Christian Brauner <brauner@kernel.org>
CC: Jan Kara <jack@suse.cz>
CC: Jeff Layton <jlayton@kernel.org>
CC: Chuck Lever <chuck.lever@oracle.com>
CC: Alexander Aring <alex.aring@gmail.com>
CC: David Laight <David.Laight@ACULAB.COM>
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-api@vger.kernel.org
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Christian Göttsche <cgzones@googlemail.com>
--
2.44.0
^ permalink raw reply
* [PATCH 1/2] fs: reorganize path_openat()
From: Stas Sergeev @ 2024-04-24 10:52 UTC (permalink / raw)
To: linux-kernel
Cc: Stas Sergeev, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240424105248.189032-1-stsp2@yandex.ru>
This patch moves the call to alloc_empty_file() below the call to
path_init(). That changes is needed for the next patch, which adds
a cred override for alloc_empty_file(). The needed cred info is only
available after the call to path_init().
No functional changes are intended by that patch.
Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
CC: Eric Biederman <ebiederm@xmission.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Christian Brauner <brauner@kernel.org>
CC: Jan Kara <jack@suse.cz>
CC: Andy Lutomirski <luto@kernel.org>
CC: David Laight <David.Laight@ACULAB.COM>
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
fs/namei.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index c5b2a25be7d0..413eef134234 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3781,23 +3781,30 @@ static struct file *path_openat(struct nameidata *nd,
{
struct file *file;
int error;
+ u64 open_flags = op->open_flag;
- file = alloc_empty_file(op->open_flag, current_cred());
- if (IS_ERR(file))
- return file;
-
- if (unlikely(file->f_flags & __O_TMPFILE)) {
- error = do_tmpfile(nd, flags, op, file);
- } else if (unlikely(file->f_flags & O_PATH)) {
- error = do_o_path(nd, flags, file);
+ if (unlikely(open_flags & (__O_TMPFILE | O_PATH))) {
+ file = alloc_empty_file(open_flags, current_cred());
+ if (IS_ERR(file))
+ return file;
+ if (open_flags & __O_TMPFILE)
+ error = do_tmpfile(nd, flags, op, file);
+ else
+ error = do_o_path(nd, flags, file);
} else {
const char *s = path_init(nd, flags);
- while (!(error = link_path_walk(s, nd)) &&
- (s = open_last_lookups(nd, file, op)) != NULL)
- ;
+ file = alloc_empty_file(open_flags, current_cred());
+ error = PTR_ERR_OR_ZERO(file);
+ if (!error) {
+ while (!(error = link_path_walk(s, nd)) &&
+ (s = open_last_lookups(nd, file, op)) != NULL)
+ ;
+ }
if (!error)
error = do_open(nd, file, op);
terminate_walk(nd);
+ if (IS_ERR(file))
+ return file;
}
if (likely(!error)) {
if (likely(file->f_mode & FMODE_OPENED))
--
2.44.0
^ permalink raw reply related
* [PATCH 2/2] openat2: add OA2_INHERIT_CRED flag
From: Stas Sergeev @ 2024-04-24 10:52 UTC (permalink / raw)
To: linux-kernel
Cc: Stas Sergeev, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, David Laight, linux-fsdevel,
linux-api, Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240424105248.189032-1-stsp2@yandex.ru>
This flag performs the open operation with the fs credentials
(fsuid, fsgid, group_info) that were in effect when dir_fd was opened.
This allows the process to pre-open some directories and then
change eUID (and all other UIDs/GIDs) to a less-privileged user,
retaining the ability to open/create files within these directories.
Design goal:
The idea is to provide a very light-weight sandboxing, where the
process, without the use of any heavy-weight techniques like chroot
within namespaces, can restrict the access to the set of pre-opened
directories.
This patch is just a first step to such sandboxing. If things go
well, in the future the same extension can be added to more syscalls.
These should include at least unlinkat(), renameat2() and the
not-yet-upstreamed setxattrat().
Security considerations:
- Only the bare minimal set of credentials is overridden:
fsuid, fsgid and group_info. The rest, for example capabilities,
are not overridden to avoid unneeded security risks.
- To avoid sandboxing escape, this patch makes sure the restricted
lookup modes are used. Namely, RESOLVE_BENEATH or RESOLVE_IN_ROOT.
- To avoid leaking creds across exec, this patch requires O_CLOEXEC
flag on a directory.
- Magic /proc symlinks are discarded, as suggested by
Andy Lutomirski <luto@kernel.org>
Use cases:
Virtual machines that deal with untrusted code, can use that
instead of a more heavy-weighted approaches.
Currently the approach is being tested on a dosemu2 VM.
Signed-off-by: Stas Sergeev <stsp2@yandex.ru>
CC: Stefan Metzmacher <metze@samba.org>
CC: Eric Biederman <ebiederm@xmission.com>
CC: Alexander Viro <viro@zeniv.linux.org.uk>
CC: Andy Lutomirski <luto@kernel.org>
CC: Christian Brauner <brauner@kernel.org>
CC: Jan Kara <jack@suse.cz>
CC: Jeff Layton <jlayton@kernel.org>
CC: Chuck Lever <chuck.lever@oracle.com>
CC: Alexander Aring <alex.aring@gmail.com>
CC: linux-fsdevel@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Christian Göttsche <cgzones@googlemail.com>
---
fs/internal.h | 2 +-
fs/namei.c | 56 ++++++++++++++++++++++++++++++++++--
fs/open.c | 10 ++++++-
include/linux/fcntl.h | 2 ++
include/uapi/linux/openat2.h | 3 ++
5 files changed, 69 insertions(+), 4 deletions(-)
diff --git a/fs/internal.h b/fs/internal.h
index 7ca738904e34..692b53b19aad 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -169,7 +169,7 @@ static inline void sb_end_ro_state_change(struct super_block *sb)
* open.c
*/
struct open_flags {
- int open_flag;
+ u64 open_flag;
umode_t mode;
int acc_mode;
int intent;
diff --git a/fs/namei.c b/fs/namei.c
index 413eef134234..aeb9f504538e 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -586,6 +586,9 @@ struct nameidata {
int dfd;
vfsuid_t dir_vfsuid;
umode_t dir_mode;
+ kuid_t dir_open_fsuid;
+ kgid_t dir_open_fsgid;
+ struct group_info *dir_open_groups;
} __randomize_layout;
#define ND_ROOT_PRESET 1
@@ -695,6 +698,8 @@ static void terminate_walk(struct nameidata *nd)
nd->depth = 0;
nd->path.mnt = NULL;
nd->path.dentry = NULL;
+ if (nd->dir_open_groups)
+ put_group_info(nd->dir_open_groups);
}
/* path_put is needed afterwards regardless of success or failure */
@@ -2414,6 +2419,9 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
get_fs_pwd(current->fs, &nd->path);
nd->inode = nd->path.dentry->d_inode;
}
+ nd->dir_open_fsuid = current_cred()->fsuid;
+ nd->dir_open_fsgid = current_cred()->fsgid;
+ nd->dir_open_groups = get_current_groups();
} else {
/* Caller must check execute permissions on the starting path component */
struct fd f = fdget_raw(nd->dfd);
@@ -2437,6 +2445,10 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
path_get(&nd->path);
nd->inode = nd->path.dentry->d_inode;
}
+ nd->dir_open_fsuid = f.file->f_cred->fsuid;
+ nd->dir_open_fsgid = f.file->f_cred->fsgid;
+ nd->dir_open_groups = get_group_info(
+ f.file->f_cred->group_info);
fdput(f);
}
@@ -3776,6 +3788,29 @@ static int do_o_path(struct nameidata *nd, unsigned flags, struct file *file)
return error;
}
+static const struct cred *openat2_override_creds(struct nameidata *nd)
+{
+ const struct cred *old_cred;
+ struct cred *override_cred;
+
+ override_cred = prepare_creds();
+ if (!override_cred)
+ return NULL;
+
+ override_cred->fsuid = nd->dir_open_fsuid;
+ override_cred->fsgid = nd->dir_open_fsgid;
+ override_cred->group_info = nd->dir_open_groups;
+
+ override_cred->non_rcu = 1;
+
+ old_cred = override_creds(override_cred);
+
+ /* override_cred() gets its own ref */
+ put_cred(override_cred);
+
+ return old_cred;
+}
+
static struct file *path_openat(struct nameidata *nd,
const struct open_flags *op, unsigned flags)
{
@@ -3793,8 +3828,23 @@ static struct file *path_openat(struct nameidata *nd,
error = do_o_path(nd, flags, file);
} else {
const char *s = path_init(nd, flags);
- file = alloc_empty_file(open_flags, current_cred());
- error = PTR_ERR_OR_ZERO(file);
+ const struct cred *old_cred = NULL;
+
+ error = 0;
+ if (open_flags & OA2_INHERIT_CRED) {
+ /* Only work with O_CLOEXEC dirs. */
+ if (!get_close_on_exec(nd->dfd))
+ error = -EPERM;
+
+ if (!error)
+ old_cred = openat2_override_creds(nd);
+ }
+ if (!error) {
+ file = alloc_empty_file(open_flags, current_cred());
+ error = PTR_ERR_OR_ZERO(file);
+ } else {
+ file = ERR_PTR(error);
+ }
if (!error) {
while (!(error = link_path_walk(s, nd)) &&
(s = open_last_lookups(nd, file, op)) != NULL)
@@ -3802,6 +3852,8 @@ static struct file *path_openat(struct nameidata *nd,
}
if (!error)
error = do_open(nd, file, op);
+ if (old_cred)
+ revert_creds(old_cred);
terminate_walk(nd);
if (IS_ERR(file))
return file;
diff --git a/fs/open.c b/fs/open.c
index ee8460c83c77..c871ff8fc6e3 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1225,7 +1225,7 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op)
* values before calling build_open_flags(), but openat2(2) checks all
* of its arguments.
*/
- if (flags & ~VALID_OPEN_FLAGS)
+ if (flags & ~VALID_OPENAT2_FLAGS)
return -EINVAL;
if (how->resolve & ~VALID_RESOLVE_FLAGS)
return -EINVAL;
@@ -1326,6 +1326,14 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op)
lookup_flags |= LOOKUP_CACHED;
}
+ if (flags & OA2_INHERIT_CRED) {
+ /* Inherit creds only with scoped look-up modes. */
+ if (!(lookup_flags & LOOKUP_IS_SCOPED))
+ return -EPERM;
+ /* Reject /proc "magic" links if inheriting creds. */
+ lookup_flags |= LOOKUP_NO_MAGICLINKS;
+ }
+
op->lookup_flags = lookup_flags;
return 0;
}
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index a332e79b3207..b71f8b162102 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -12,6 +12,8 @@
FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
+#define VALID_OPENAT2_FLAGS (VALID_OPEN_FLAGS | OA2_INHERIT_CRED)
+
/* List of all valid flags for the how->resolve argument: */
#define VALID_RESOLVE_FLAGS \
(RESOLVE_NO_XDEV | RESOLVE_NO_MAGICLINKS | RESOLVE_NO_SYMLINKS | \
diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h
index a5feb7604948..cdd676a10b62 100644
--- a/include/uapi/linux/openat2.h
+++ b/include/uapi/linux/openat2.h
@@ -40,4 +40,7 @@ struct open_how {
return -EAGAIN if that's not
possible. */
+/* openat2-specific flags go to upper 4 bytes. */
+#define OA2_INHERIT_CRED (1ULL << 32)
+
#endif /* _UAPI_LINUX_OPENAT2_H */
--
2.44.0
^ permalink raw reply related
* Re: [PATCH v2 0/2] implement OA2_INHERIT_CRED flag for openat2()
From: stsp @ 2024-04-24 10:57 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <4D2A1543-273F-417F-921B-E9F994FBF2E8@amacapital.net>
23.04.2024 19:44, Andy Lutomirski пишет:
>> On Apr 23, 2024, at 4:02 AM, Stas Sergeev <stsp2@yandex.ru> wrote:
>>
>> This patch-set implements the OA2_INHERIT_CRED flag for openat2() syscall.
>> It is needed to perform an open operation with the creds that were in
>> effect when the dir_fd was opened. This allows the process to pre-open
>> some dirs and switch eUID (and other UIDs/GIDs) to the less-privileged
>> user, while still retaining the possibility to open/create files within
>> the pre-opened directory set.
> I like the concept, as it’s a sort of move toward a capability system. But I think that making a dirfd into this sort of capability would need to be much more explicit. Right now, any program could do this entirely by accident, and applying OA2_INHERIT_CRED to an fd fished out of /proc seems hazardous.
While I still don't quite understand
the threat of /proc symlinks, I posted
v4 which disallows them.
> So perhaps if an open file description for a directory could have something like FMODE_CRED, and if OA2_INHERIT_CRED also blocked .., magic links, symlinks to anywhere above the dirfd (or maybe all symlinks) and absolute path lookups, then this would be okay.
So I think this all is now done.
> Also, there are lots of ways that f_cred could be relevant: fsuid/fsgid, effective capabilities and security labels. And it gets more complex if this ever gets extended to support connecting or sending to a socket or if someone opens a device node. Does CAP_SYS_ADMIN carry over?
Not any more, nothin is carried but
fsuid, fsgid, groupinfo.
Thank you.
^ permalink raw reply
* Re: [PATCH v4 0/2] implement OA2_INHERIT_CRED flag for openat2()
From: Christian Brauner @ 2024-04-24 16:09 UTC (permalink / raw)
To: Stas Sergeev
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240424105248.189032-1-stsp2@yandex.ru>
On Wed, Apr 24, 2024 at 01:52:46PM +0300, Stas Sergeev wrote:
> This patch-set implements the OA2_INHERIT_CRED flag for openat2() syscall.
> It is needed to perform an open operation with the creds that were in
> effect when the dir_fd was opened. This allows the process to pre-open
> some dirs and switch eUID (and other UIDs/GIDs) to the less-privileged
> user, while still retaining the possibility to open/create files within
> the pre-opened directory set.
>
> The sand-boxing is security-oriented: symlinks leading outside of a
> sand-box are rejected. /proc magic links are rejected.
> The more detailed description (including security considerations)
> is available in the log messages of individual patches.
>
> Changes in v4:
> - add optimizations suggested by David Laight <David.Laight@ACULAB.COM>
> - move security checks to build_open_flags()
> - force RESOLVE_NO_MAGICLINKS as suggested by Andy Lutomirski <luto@kernel.org>
>
> Changes in v3:
> - partially revert v2 changes to avoid overriding capabilities.
> Only the bare minimum is overridden: fsuid, fsgid and group_info.
> Document the fact the full cred override is unwanted, as it may
> represent an unneeded security risk.
>
> Changes in v2:
> - capture full struct cred instead of just fsuid/fsgid.
> Suggested by Stefan Metzmacher <metze@samba.org>
This smells ripe enough to serve as an attack vector in non-obvious
ways. And in general this has the potential to confuse the hell out
unsuspecting userspace. They can now suddenly get sent such
special-sauce files such as this that they have no way of recognizing as
there's neither an FMODE_* flag nor is the OA2_* flag recorded so it's
not available in F_GETFL.
There's not even a way to restrict that new flag because no LSM ever
sees it. So that behavior might break LSM assumptions as well.
And it is effectively usable to steal credentials. If process A opens a
directory with uid/gid 0 then sends that directory fd via AF_UNIX or
something to process B then process B can inherit the uid/gid of process
A by specifying OA2_* with no way for process A to prevent this - not
even through an LSM.
The permission checking model that we have right now is already baroque.
I see zero reason to add more complexity for the sake of "lightweight
sandboxing". We have LSMs and namespaces for stuff like this.
NAK.
^ permalink raw reply
* Re: [PATCH v4 0/2] implement OA2_INHERIT_CRED flag for openat2()
From: stsp @ 2024-04-24 17:50 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240424-schummeln-zitieren-9821df7cbd49@brauner>
24.04.2024 19:09, Christian Brauner пишет:
> This smells ripe enough to serve as an attack vector in non-obvious
> ways. And in general this has the potential to confuse the hell out
> unsuspecting userspace.
Unsuspecting user-space will simply
not use this flag. What do you mean?
> They can now suddenly get sent such
> special-sauce files
There are no any special files.
This flag helps you to open a file on
which you currently have no perms
to open, but had those in the past.
> such as this that they have no way of recognizing as
> there's neither an FMODE_* flag nor is the OA2_* flag recorded so it's
> not available in F_GETFL.
>
> There's not even a way to restrict that new flag because no LSM ever
> sees it. So that behavior might break LSM assumptions as well.
>
> And it is effectively usable to steal credentials. If process A opens a
> directory with uid/gid 0 then sends that directory fd via AF_UNIX or
> something to process B then process B can inherit the uid/gid of process
No, it doesn't inherit anything.
The inheritance happens only for
a duration of an open() call, helping
open() to succeed. The creds are
reverted when open() completed.
The only theoretically possible attack
would be to open some file you'd never
intended to open. Also note that a
very minimal sed of creds is overridden:
fsuid, fsgid, groupinfo.
> A by specifying OA2_* with no way for process A to prevent this - not
> even through an LSM.
If process B doesn't use that flag, it
inherits nothing, no matter what process
A did or passed via a socket.
So an unaware process that doesn't
use that flag, is completely unaffected.
> The permission checking model that we have right now is already baroque.
> I see zero reason to add more complexity for the sake of "lightweight
> sandboxing". We have LSMs and namespaces for stuff like this.
>
> NAK.
I don't think it is fair to say NAK
without actually reading the patch
or asking its author for clarifications.
Even though you didn't ask, I provided
my clarifications above, as I find that
a polite action.
^ permalink raw reply
* Re: [PATCH v2] usb: gadget: f_uac2: Expose all string descriptors through configfs.
From: Chris Wulff @ 2024-04-24 19:01 UTC (permalink / raw)
To: Pavel Hofman, linux-usb@vger.kernel.org
Cc: Greg Kroah-Hartman, James Gruber, Lee Jones,
linux-kernel@vger.kernel.org, linux-api@vger.kernel.org
In-Reply-To: <9b40e148-f3eb-f8f5-bf2d-37a0a0629417@ivitera.com>
>From: Pavel Hofman <pavel.hofman@ivitera.com>
>Sent: Wednesday, April 24, 2024 3:55 AM
>>>> + p_it_name playback input terminal name
>>>> + p_ot_name playback output terminal name
>>>> + p_fu_name playback function unit name
>>>> + p_alt0_name playback alt mode 0 name
>>>> + p_alt1_name playback alt mode 1 name
>>>
>>> Nacked-by: Pavel Hofman <pavel.hofman@ivitera.com>
>>>
>>> I am not sure adding a numbered parameter for every additional alt mode
>>> is a way to go for the future. I am not that much concerned about UAC1,
>>> but IMO (at least) in UAC2 the configuration method should be flexible
>>> for more alt setttings. I can see use cases with many more altsettings.
>>>
>>> My proposal for adding more alt settings
>>> https://lore.kernel.org/linux-usb/35be4668-58d3-894a-72cf-de1afaacae45@ivitera.com/
>>> suggested using lists to existing parameters where each item would
>>> correspond to the alt setting of the same index (+1). That would allow
>>> using more altsettings easily, without having to add parameters to the
>>> source code and adding configfs params. I received no feedback. I do not
>>> push the param list proposal, but I am convinced an acceptable solution
>>> should be discussed thoroughly by the UAC2 gadget stakeholders.
>>>
>>> I am afraid that once p_alt1_name/c_alt1_name params are accepted, there
>>> will be no way back because subsequent removal of configfs params could
>>> be viewed as a regression for users.
>>
>> I have been thinking about this as well. The alt names are slightly different than the rest of the settings
>> since they also include alt mode 0. I was thinking p/c_alt1_name could be expanded to the array so
>> that the entries line up with the other settings and don't have an extra entry for alt 0. Perhaps a different
>> name would make more sense.
>>
>> Along those lines, I didn't see any gadget drivers using an array of strings for anything, which is also why
>> I didn't try to do anything here that merged alt0/1 names into an array. If we were to do an array of strings
>> I'm not sure what the best separator would be. Maybe ";"? The rates array uses ",".
>>
>> This patch only exposes the existing strings to make them configurable, but I don't want to do anything
>> that would preclude a nice interface for extra alt modes.
>>
>
>Thanks a lot for your response. Please can you take a look at
>https://lore.kernel.org/linux-usb/72e9b581-4a91-2319-cb9f-0bcb370f34a1@ivitera.com/T/ ?
>
>If the params in the upper level were to stand as defaults for the
>altsettings (and for the existing altsetting 1 if no specific altset
>subdir configs were given), maybe the naming xxx_alt1_xxx could become a
>bit confusing. E.g. p_altx_name or p_alt_non0_name?
I am in favor of the subdirs for alt mode settings, with the main config options acting as the default/single
configuration as it is today.
I can change these patches from c/p_alt1_name to c/p_altx_name if nobody objects to that, or I could remove
the alt name from this patch set if anyone thinks this needs more discussion. I don't actually need to set
the alt name for my use case, but included it for completeness.
-- Chris Wulff
^ permalink raw reply
* [PATCH v3] usb: gadget: f_fs: add capability for dfu run-time descriptor
From: Chris Wulff @ 2024-04-24 22:14 UTC (permalink / raw)
To: linux-usb@vger.kernel.org
Cc: Greg Kroah-Hartman, Christian Brauner, Paul Cercueil, Jan Kara,
Jeff Layton, Dmitry Antipov, David Sands,
linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
Chris Wulff
From: David Sands <david.sands@biamp.com>
Add the ability for FunctionFS driver to be able to create DFU Run-Time
descriptors.
Signed-off-by: David Sands <david.sands@biamp.com>
Co-developed-by: Chris Wulff <chris.wulff@biamp.com>
Signed-off-by: Chris Wulff <chris.wulff@biamp.com>
---
v3: Documentation, additional constants and constant order fixed
v2: https://lore.kernel.org/linux-usb/CO1PR17MB54198D086B61F7392FA9075FE10E2@CO1PR17MB5419.namprd17.prod.outlook.com/
v1: https://lore.kernel.org/linux-usb/CO1PR17MB5419AC3907C74E28D80C5021E1082@CO1PR17MB5419.namprd17.prod.outlook.com/
drivers/usb/gadget/function/f_fs.c | 12 ++++++++++--
include/uapi/linux/usb/ch9.h | 9 +++++++--
include/uapi/linux/usb/functionfs.h | 25 +++++++++++++++++++++++++
3 files changed, 42 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index bffbc1dc651f..4cc3f3601cf0 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -2467,7 +2467,7 @@ typedef int (*ffs_os_desc_callback)(enum ffs_os_desc_type entity,
static int __must_check ffs_do_single_desc(char *data, unsigned len,
ffs_entity_callback entity,
- void *priv, int *current_class)
+ void *priv, int *current_class, int *current_subclass)
{
struct usb_descriptor_header *_ds = (void *)data;
u8 length;
@@ -2524,6 +2524,7 @@ static int __must_check ffs_do_single_desc(char *data, unsigned len,
if (ds->iInterface)
__entity(STRING, ds->iInterface);
*current_class = ds->bInterfaceClass;
+ *current_subclass = ds->bInterfaceSubClass;
}
break;
@@ -2548,6 +2549,12 @@ static int __must_check ffs_do_single_desc(char *data, unsigned len,
if (length != sizeof(struct ccid_descriptor))
goto inv_length;
break;
+ } else if (*current_class == USB_CLASS_APP_SPEC &&
+ *current_subclass == USB_SUBCLASS_DFU) {
+ pr_vdebug("dfu functional descriptor\n");
+ if (length != sizeof(struct usb_dfu_functional_descriptor))
+ goto inv_length;
+ break;
} else {
pr_vdebug("unknown descriptor: %d for class %d\n",
_ds->bDescriptorType, *current_class);
@@ -2610,6 +2617,7 @@ static int __must_check ffs_do_descs(unsigned count, char *data, unsigned len,
const unsigned _len = len;
unsigned long num = 0;
int current_class = -1;
+ int current_subclass = -1;
for (;;) {
int ret;
@@ -2629,7 +2637,7 @@ static int __must_check ffs_do_descs(unsigned count, char *data, unsigned len,
return _len - len;
ret = ffs_do_single_desc(data, len, entity, priv,
- ¤t_class);
+ ¤t_class, ¤t_subclass);
if (ret < 0) {
pr_debug("%s returns %d\n", __func__, ret);
return ret;
diff --git a/include/uapi/linux/usb/ch9.h b/include/uapi/linux/usb/ch9.h
index 44d73ba8788d..7f155fba0c1f 100644
--- a/include/uapi/linux/usb/ch9.h
+++ b/include/uapi/linux/usb/ch9.h
@@ -254,6 +254,9 @@ struct usb_ctrlrequest {
#define USB_DT_DEVICE_CAPABILITY 0x10
#define USB_DT_WIRELESS_ENDPOINT_COMP 0x11
#define USB_DT_WIRE_ADAPTER 0x21
+/* From USB Device Firmware Upgrade Specification, Revision 1.1 */
+#define USB_DT_DFU_FUNCTIONAL 0x21
+/* these are from the Wireless USB spec */
#define USB_DT_RPIPE 0x22
#define USB_DT_CS_RADIO_CONTROL 0x23
/* From the T10 UAS specification */
@@ -263,6 +266,7 @@ struct usb_ctrlrequest {
/* From the USB 3.1 spec */
#define USB_DT_SSP_ISOC_ENDPOINT_COMP 0x31
+
/* Conventional codes for class-specific descriptors. The convention is
* defined in the USB "Common Class" Spec (3.11). Individual class specs
* are authoritative for their usage, not the "common class" writeup.
@@ -329,9 +333,10 @@ struct usb_device_descriptor {
#define USB_CLASS_USB_TYPE_C_BRIDGE 0x12
#define USB_CLASS_MISC 0xef
#define USB_CLASS_APP_SPEC 0xfe
-#define USB_CLASS_VENDOR_SPEC 0xff
+#define USB_SUBCLASS_DFU 0x01
-#define USB_SUBCLASS_VENDOR_SPEC 0xff
+#define USB_CLASS_VENDOR_SPEC 0xff
+#define USB_SUBCLASS_VENDOR_SPEC 0xff
/*-------------------------------------------------------------------------*/
diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
index 9f88de9c3d66..6d2061500184 100644
--- a/include/uapi/linux/usb/functionfs.h
+++ b/include/uapi/linux/usb/functionfs.h
@@ -37,6 +37,31 @@ struct usb_endpoint_descriptor_no_audio {
__u8 bInterval;
} __attribute__((packed));
+/**
+ * struct usb_dfu_functional_descriptor - DFU Functional descriptor
+ * @bLength: Size of the descriptor (bytes)
+ * @bDescriptorType: USB_DT_DFU_FUNCTIONAL
+ * @bmAttributes: DFU attributes
+ * @wDetachTimeOut: Maximum time to wait after DFU_DETACH (ms, le16)
+ * @wTransferSize: Maximum number of bytes per control-write (le16)
+ * @bcdDFUVersion: DFU Spec version (BCD, le16)
+ */
+struct usb_dfu_functional_descriptor {
+ __u8 bLength;
+ __u8 bDescriptorType;
+ __u8 bmAttributes;
+ __le16 wDetachTimeOut;
+ __le16 wTransferSize;
+ __le16 bcdDFUVersion;
+} __attribute__ ((packed));
+
+/* from DFU functional descriptor bmAttributes */
+#define DFU_FUNC_ATT_WILL_DETACH (1 << 3)
+#define DFU_FUNC_ATT_MANIFEST_TOLERANT (1 << 2)
+#define DFU_FUNC_ATT_CAN_UPLOAD (1 << 1)
+#define DFU_FUNC_ATT_CAN_DOWNLOAD (1 << 0)
+
+
struct usb_functionfs_descs_head_v2 {
__le32 magic;
__le32 length;
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v2 0/2] implement OA2_INHERIT_CRED flag for openat2()
From: Andy Lutomirski @ 2024-04-25 0:43 UTC (permalink / raw)
To: stsp
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche, Aleksa Sarai
In-Reply-To: <0e2e48be-86a8-418c-95b1-e8ca17469198@yandex.ru>
On Wed, Apr 24, 2024 at 3:57 AM stsp <stsp2@yandex.ru> wrote:
>
> 23.04.2024 19:44, Andy Lutomirski пишет:
> >> On Apr 23, 2024, at 4:02 AM, Stas Sergeev <stsp2@yandex.ru> wrote:
> >>
> >> This patch-set implements the OA2_INHERIT_CRED flag for openat2() syscall.
> >> It is needed to perform an open operation with the creds that were in
> >> effect when the dir_fd was opened. This allows the process to pre-open
> >> some dirs and switch eUID (and other UIDs/GIDs) to the less-privileged
> >> user, while still retaining the possibility to open/create files within
> >> the pre-opened directory set.
> > I like the concept, as it’s a sort of move toward a capability system. But I think that making a dirfd into this sort of capability would need to be much more explicit. Right now, any program could do this entirely by accident, and applying OA2_INHERIT_CRED to an fd fished out of /proc seems hazardous.
>
> While I still don't quite understand
> the threat of /proc symlinks, I posted
> v4 which disallows them.
>
I like that, but you're blocking it the wrong way. My concern is that
someone does dfd = open("/proc/PID/fd/3") and then openat(dfd, ...,
OA2_INHERIT_CRED); IIRC open("/proc/PID/fd/3") is extremely magical
and returns the _same open file description_ (struct file) as PID's fd
3.
> > So perhaps if an open file description for a directory could have something like FMODE_CRED, and if OA2_INHERIT_CRED also blocked .., magic links, symlinks to anywhere above the dirfd (or maybe all symlinks) and absolute path lookups, then this would be okay.
>
> So I think this all is now done.
But you missed the FMODE_CRED part!
So here's the problem: right now, in current Linux, a dirfd pointing
to a directory that you can open anyway doesn't convey any new powers.
So, if I'm a regular program, and I do open("/etc", O_PATH), I get an
fd. And if I get an fd pointing at /etc from somewhere else, I get
the same thing (possibly with different f_cred, but f_cred is largely
a hack to restrict things that would otherwise be insecure because
they were designed a bit wrong from the beginning).
But, with your patch, these fds suddenly convey a very strong
privilege: that of their f_cred *over the entire subtree to which they
refer*. And you can attack it using exactly your intended use case:
if any program opens a dirfd and then drops privileges, well, oops, it
didn't actually fully drop privilege.
So I think that, if this whole concept has any chance of working well,
it needs to be opt-in *at the time of the original open*. So a
privilege-carrying open would be an entirely new option like
O_CAPTURE_CREDS or FMODE_CREDS. And OA2_INHERIT_CREDS is rejected if
the dirfd doesn't have that special mode.
^ permalink raw reply
* Re: [PATCH v2 0/2] implement OA2_INHERIT_CRED flag for openat2()
From: Al Viro @ 2024-04-25 1:43 UTC (permalink / raw)
To: Andy Lutomirski
Cc: stsp, linux-kernel, Stefan Metzmacher, Eric Biederman,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche, Aleksa Sarai
In-Reply-To: <CALCETrWswr5jAzD9BkdCqLX=d8vReO8O9dVmZfL7HXdvwkft9g@mail.gmail.com>
On Wed, Apr 24, 2024 at 05:43:02PM -0700, Andy Lutomirski wrote:
> I like that, but you're blocking it the wrong way. My concern is that
> someone does dfd = open("/proc/PID/fd/3") and then openat(dfd, ...,
> OA2_INHERIT_CRED); IIRC open("/proc/PID/fd/3") is extremely magical
> and returns the _same open file description_ (struct file) as PID's fd
> 3.
No, it doesn't. We could implement that, but if we do that'll be
*not* a part of procfs and it's going to be limited to current task
only.
There are two different variants of /dev/fd/* semantics - one is
"opening /dev/fd/42 is an equivalent of dup(42)", another is
"opening /dev/fd/42 is an equivalent of opening the same fs object
that is currently accessed via descriptor 42". Linux is doing the
latter, and we can't switch - that would break a lot of userland
software, including a lot of scripts.
I'm not saying I like the series, but this particular objection is bogus -
open via procfs symlinks is *not* an equivalent of dup() and that is not
going to change.
^ permalink raw reply
* Re: [PATCH 2/2] openat2: add OA2_INHERIT_CRED flag
From: Al Viro @ 2024-04-25 2:31 UTC (permalink / raw)
To: Stas Sergeev
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Andy Lutomirski,
Christian Brauner, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240424105248.189032-3-stsp2@yandex.ru>
On Wed, Apr 24, 2024 at 01:52:48PM +0300, Stas Sergeev wrote:
> @@ -3793,8 +3828,23 @@ static struct file *path_openat(struct nameidata *nd,
> error = do_o_path(nd, flags, file);
> } else {
> const char *s = path_init(nd, flags);
> - file = alloc_empty_file(open_flags, current_cred());
> - error = PTR_ERR_OR_ZERO(file);
> + const struct cred *old_cred = NULL;
> +
> + error = 0;
> + if (open_flags & OA2_INHERIT_CRED) {
> + /* Only work with O_CLOEXEC dirs. */
> + if (!get_close_on_exec(nd->dfd))
> + error = -EPERM;
> +
> + if (!error)
> + old_cred = openat2_override_creds(nd);
> + }
> + if (!error) {
> + file = alloc_empty_file(open_flags, current_cred());
Consider the following, currently absolutely harmless situation:
* process is owned by luser:students.
* descriptor 69 refers to root-opened root directory (O_RDONLY)
What's the expected result of
fcntl(69, F_SEFTD, O_CLOEXEC);
opening "etc/shadow" with dirfd equal to 69 and your flag given
subsequent read() from the resulting descriptor?
At which point will the kernel say "go fuck yourself, I'm not letting you
read that file", provided that attacker passes that new flag of yours?
As a bonus question, how about opening it for _write_, seeing that this
is an obvious instant roothole?
Again, currently the setup that has a root-opened directory in descriptor
table of a non-root process is safe.
Incidentally, suppose you have the same process run with stdin opened
(r/o) by root. F_SETFD it to O_CLOEXEC, then use your open with
dirfd being 0, pathname - "" and flags - O_RDWR.
AFAICS, without an explicit opt-in by the original opener it's
a non-starter, and TBH I doubt that even with such opt-in (FMODE_CRED,
whatever) it would be a good idea - it gives too much.
NAKed-by: Al Viro <viro@zeniv.linux.org.uk>
^ permalink raw reply
* Re: [PATCH v2 0/2] implement OA2_INHERIT_CRED flag for openat2()
From: Andy Lutomirski @ 2024-04-25 3:31 UTC (permalink / raw)
To: Al Viro
Cc: stsp, linux-kernel, Stefan Metzmacher, Eric Biederman,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche, Aleksa Sarai
In-Reply-To: <20240425014358.GG2118490@ZenIV>
On Wed, Apr 24, 2024 at 6:44 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Apr 24, 2024 at 05:43:02PM -0700, Andy Lutomirski wrote:
>
> > I like that, but you're blocking it the wrong way. My concern is that
> > someone does dfd = open("/proc/PID/fd/3") and then openat(dfd, ...,
> > OA2_INHERIT_CRED); IIRC open("/proc/PID/fd/3") is extremely magical
> > and returns the _same open file description_ (struct file) as PID's fd
> > 3.
>
> No, it doesn't. We could implement that, but if we do that'll be
> *not* a part of procfs and it's going to be limited to current task
> only.
Egads -- why would we want to implement that? In the apparently
incorrect model in my head, Linux's behavior was ridiculous and only
made sense for some historical reason. But I wonder why I thought
that.
Diving a tiny bit down the rabbit hole, I have a copy of TLPI on my
bookshelf, and I bought it quite a long time ago and read a bunch of
it when I got it, and my copy is *wrong*! Section 5.11 has the
behavior of /dev/fd very clearly documented as working like dup().
And here it is: erratum 107. Whoopsies!
https://man7.org/tlpi/errata/index.html
Anyway, I retract that particular objection to the series. But I
wouldn't be shocked if one can break a normal modern systemd using the
patchset -- systemd does all kinds of fun things involving passing
file descriptors around.
--Andy
^ permalink raw reply
* Re: [PATCH 2/2] openat2: add OA2_INHERIT_CRED flag
From: stsp @ 2024-04-25 7:24 UTC (permalink / raw)
To: Al Viro
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Andy Lutomirski,
Christian Brauner, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240425023127.GH2118490@ZenIV>
25.04.2024 05:31, Al Viro пишет:
> Consider the following, currently absolutely harmless situation:
> * process is owned by luser:students.
> * descriptor 69 refers to root-opened root directory (O_RDONLY)
> What's the expected result of
> fcntl(69, F_SEFTD, O_CLOEXEC);
> opening "etc/shadow" with dirfd equal to 69 and your flag given
> subsequent read() from the resulting descriptor?
>
> At which point will the kernel say "go fuck yourself, I'm not letting you
> read that file", provided that attacker passes that new flag of yours?
>
> As a bonus question, how about opening it for _write_, seeing that this
> is an obvious instant roothole?
>
> Again, currently the setup that has a root-opened directory in descriptor
> table of a non-root process is safe.
>
> Incidentally, suppose you have the same process run with stdin opened
> (r/o) by root. F_SETFD it to O_CLOEXEC, then use your open with
> dirfd being 0, pathname - "" and flags - O_RDWR.
Ok, F_SETFD, how simple. :(
> AFAICS, without an explicit opt-in by the original opener it's
> a non-starter, and TBH I doubt that even with such opt-in (FMODE_CRED,
> whatever) it would be a good idea - it gives too much.
Yes, which is why I am quite sceptical
to this FMODE_CRED idea.
Please note that my O_CLOEXEC check
actually meant to check that exactly this
process have opened the dir. It just didn't
happen that way, as you pointed.
Can I replace the O_CLOEXEC check with
some explicit check that makes sure the
fd was opened by exactly that process?
^ permalink raw reply
* Re: [PATCH 1/2] fs: reorganize path_openat()
From: kernel test robot @ 2024-04-25 8:13 UTC (permalink / raw)
To: Stas Sergeev
Cc: oe-lkp, lkp, Eric Biederman, Alexander Viro, Christian Brauner,
Jan Kara, Andy Lutomirski, David Laight, linux-fsdevel,
linux-kernel, Stas Sergeev, Stefan Metzmacher, Jeff Layton,
Chuck Lever, Alexander Aring, linux-api, Paolo Bonzini,
Christian Göttsche, oliver.sang
In-Reply-To: <20240424105248.189032-2-stsp2@yandex.ru>
Hello,
kernel test robot noticed "BUG:sleeping_function_called_from_invalid_context_at_include/linux/sched/mm.h" on:
commit: 831d3c6cc6f05873e33f4aaebafbb9c27618ea0b ("[PATCH 1/2] fs: reorganize path_openat()")
url: https://github.com/intel-lab-lkp/linux/commits/Stas-Sergeev/fs-reorganize-path_openat/20240424-185527
base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git 9d1ddab261f3e2af7c384dc02238784ce0cf9f98
patch link: https://lore.kernel.org/all/20240424105248.189032-2-stsp2@yandex.ru/
patch subject: [PATCH 1/2] fs: reorganize path_openat()
in testcase: boot
compiler: clang-17
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G
(please refer to attached dmesg/kmsg for entire log/backtrace)
+-------------------------------------------------------------------------------+------------+------------+
| | 9d1ddab261 | 831d3c6cc6 |
+-------------------------------------------------------------------------------+------------+------------+
| boot_successes | 6 | 0 |
| boot_failures | 0 | 6 |
| BUG:sleeping_function_called_from_invalid_context_at_include/linux/sched/mm.h | 0 | 6 |
+-------------------------------------------------------------------------------+------------+------------+
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202404251525.39b4af4e-lkp@intel.com
[ 0.591465][ T33] BUG: sleeping function called from invalid context at include/linux/sched/mm.h:315
[ 0.592508][ T33] in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 33, name: kworker/u8:2
[ 0.593515][ T33] preempt_count: 0, expected: 0
[ 0.594071][ T33] RCU nest depth: 1, expected: 0
[ 0.594633][ T33] CPU: 0 PID: 33 Comm: kworker/u8:2 Not tainted 6.9.0-rc5-00037-g831d3c6cc6f0 #1
[ 0.595637][ T33] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 0.596216][ T33] Workqueue: async async_run_entry_fn
[ 0.596216][ T33] Call Trace:
[ 0.596216][ T33] <TASK>
[ 0.596216][ T33] dump_stack_lvl (lib/dump_stack.c:116)
[ 0.596216][ T33] __might_resched (kernel/sched/core.c:10198)
[ 0.596216][ T33] kmem_cache_alloc (include/linux/kernel.h:73 include/linux/sched/mm.h:315 mm/slub.c:3746 mm/slub.c:3827 mm/slub.c:3852)
[ 0.596216][ T33] alloc_empty_file (fs/file_table.c:203)
[ 0.596216][ T33] path_openat (fs/namei.c:3796)
[ 0.596216][ T33] do_filp_open (fs/namei.c:3833)
[ 0.596216][ T33] file_open_name (fs/open.c:1352)
[ 0.596216][ T33] filp_open (fs/open.c:1371)
[ 0.596216][ T33] do_name (init/initramfs.c:373)
[ 0.596216][ T33] flush_buffer (init/initramfs.c:452 init/initramfs.c:464)
[ 0.596216][ T33] ? __pfx_flush_buffer (init/initramfs.c:458)
[ 0.596216][ T33] __gunzip (lib/decompress_inflate.c:161)
[ 0.596216][ T33] ? __pfx_nofill (lib/decompress_inflate.c:37)
[ 0.596216][ T33] unpack_to_rootfs (init/initramfs.c:520)
[ 0.596216][ T33] ? __pfx_error (init/initramfs.c:59)
[ 0.596216][ T33] do_populate_rootfs (init/initramfs.c:714)
[ 0.596216][ T33] async_run_entry_fn (kernel/async.c:136)
[ 0.596216][ T33] process_scheduled_works (kernel/workqueue.c:3259)
[ 0.596216][ T33] worker_thread (include/linux/list.h:373 kernel/workqueue.c:955 kernel/workqueue.c:3417)
[ 0.596216][ T33] ? __pfx_worker_thread (kernel/workqueue.c:3362)
[ 0.596216][ T33] kthread (kernel/kthread.c:390)
[ 0.596216][ T33] ? __pfx_kthread (kernel/kthread.c:341)
[ 0.596216][ T33] ret_from_fork (arch/x86/kernel/process.c:153)
[ 0.596216][ T33] ? __pfx_kthread (kernel/kthread.c:341)
[ 0.596216][ T33] ret_from_fork_asm (arch/x86/entry/entry_64.S:257)
[ 0.596216][ T33] </TASK>
[ 1.603321][ T33] BUG: sleeping function called from invalid context at include/linux/sched/mm.h:315
[ 1.604448][ T33] in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 33, name: kworker/u8:2
[ 1.605466][ T33] preempt_count: 0, expected: 0
[ 1.606028][ T33] RCU nest depth: 1, expected: 0
[ 1.606599][ T33] CPU: 0 PID: 33 Comm: kworker/u8:2 Tainted: G W 6.9.0-rc5-00037-g831d3c6cc6f0 #1
[ 1.607761][ T33] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 1.608136][ T33] Workqueue: async async_run_entry_fn
[ 1.608136][ T33] Call Trace:
[ 1.608136][ T33] <TASK>
[ 1.608136][ T33] dump_stack_lvl (lib/dump_stack.c:116)
[ 1.608136][ T33] __might_resched (kernel/sched/core.c:10198)
[ 1.608136][ T33] kmem_cache_alloc (include/linux/kernel.h:73 include/linux/sched/mm.h:315 mm/slub.c:3746 mm/slub.c:3827 mm/slub.c:3852)
[ 1.608136][ T33] alloc_empty_file (fs/file_table.c:203)
[ 1.608136][ T33] path_openat (fs/namei.c:3796)
[ 1.608136][ T33] do_filp_open (fs/namei.c:3833)
[ 1.608136][ T33] file_open_name (fs/open.c:1352)
[ 1.608136][ T33] filp_open (fs/open.c:1371)
[ 1.608136][ T33] do_name (init/initramfs.c:373)
[ 1.608136][ T33] flush_buffer (init/initramfs.c:452 init/initramfs.c:464)
[ 1.608136][ T33] ? __pfx_flush_buffer (init/initramfs.c:458)
[ 1.608136][ T33] __gunzip (lib/decompress_inflate.c:161)
[ 1.608136][ T33] ? __pfx_nofill (lib/decompress_inflate.c:37)
[ 1.608136][ T33] unpack_to_rootfs (init/initramfs.c:520)
[ 1.608136][ T33] ? __pfx_error (init/initramfs.c:59)
[ 1.608136][ T33] do_populate_rootfs (init/initramfs.c:714)
[ 1.608136][ T33] async_run_entry_fn (kernel/async.c:136)
[ 1.608136][ T33] process_scheduled_works (kernel/workqueue.c:3259)
[ 1.608136][ T33] worker_thread (include/linux/list.h:373 kernel/workqueue.c:955 kernel/workqueue.c:3417)
[ 1.608136][ T33] ? __pfx_worker_thread (kernel/workqueue.c:3362)
[ 1.608136][ T33] kthread (kernel/kthread.c:390)
[ 1.608136][ T33] ? __pfx_kthread (kernel/kthread.c:341)
[ 1.608136][ T33] ret_from_fork (arch/x86/kernel/process.c:153)
[ 1.608136][ T33] ? __pfx_kthread (kernel/kthread.c:341)
[ 1.608136][ T33] ret_from_fork_asm (arch/x86/entry/entry_64.S:257)
[ 1.608136][ T33] </TASK>
[ 2.602317][ T33] BUG: sleeping function called from invalid context at include/linux/sched/mm.h:315
[ 2.603414][ T33] in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 33, name: kworker/u8:2
[ 2.604433][ T33] preempt_count: 0, expected: 0
[ 2.604985][ T33] RCU nest depth: 1, expected: 0
[ 2.605547][ T33] CPU: 0 PID: 33 Comm: kworker/u8:2 Tainted: G W 6.9.0-rc5-00037-g831d3c6cc6f0 #1
[ 2.606689][ T33] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 2.607825][ T33] Workqueue: async async_run_entry_fn
[ 2.608140][ T33] Call Trace:
[ 2.608140][ T33] <TASK>
[ 2.608140][ T33] dump_stack_lvl (lib/dump_stack.c:116)
[ 2.608140][ T33] __might_resched (kernel/sched/core.c:10198)
[ 2.608140][ T33] kmem_cache_alloc (include/linux/kernel.h:73 include/linux/sched/mm.h:315 mm/slub.c:3746 mm/slub.c:3827 mm/slub.c:3852)
[ 2.608140][ T33] alloc_empty_file (fs/file_table.c:203)
[ 2.608140][ T33] path_openat (fs/namei.c:3796)
[ 2.608140][ T33] do_filp_open (fs/namei.c:3833)
[ 2.608140][ T33] file_open_name (fs/open.c:1352)
[ 2.608140][ T33] filp_open (fs/open.c:1371)
[ 2.608140][ T33] do_name (init/initramfs.c:373)
[ 2.608140][ T33] flush_buffer (init/initramfs.c:452 init/initramfs.c:464)
[ 2.608140][ T33] ? __pfx_flush_buffer (init/initramfs.c:458)
[ 2.608140][ T33] __gunzip (lib/decompress_inflate.c:161)
[ 2.608140][ T33] ? __pfx_nofill (lib/decompress_inflate.c:37)
[ 2.608140][ T33] unpack_to_rootfs (init/initramfs.c:520)
[ 2.608140][ T33] ? __pfx_error (init/initramfs.c:59)
[ 2.608140][ T33] do_populate_rootfs (init/initramfs.c:714)
[ 2.608140][ T33] async_run_entry_fn (kernel/async.c:136)
[ 2.608140][ T33] process_scheduled_works (kernel/workqueue.c:3259)
[ 2.608140][ T33] worker_thread (include/linux/list.h:373 kernel/workqueue.c:955 kernel/workqueue.c:3417)
[ 2.608140][ T33] ? __pfx_worker_thread (kernel/workqueue.c:3362)
[ 2.608140][ T33] kthread (kernel/kthread.c:390)
[ 2.608140][ T33] ? __pfx_kthread (kernel/kthread.c:341)
[ 2.608140][ T33] ret_from_fork (arch/x86/kernel/process.c:153)
[ 2.608140][ T33] ? __pfx_kthread (kernel/kthread.c:341)
[ 2.608140][ T33] ret_from_fork_asm (arch/x86/entry/entry_64.S:257)
[ 2.608140][ T33] </TASK>
[ 3.648001][ T33] BUG: sleeping function called from invalid context at include/linux/sched/mm.h:315
[ 3.649103][ T33] in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 33, name: kworker/u8:2
[ 3.650109][ T33] preempt_count: 0, expected: 0
[ 3.650660][ T33] RCU nest depth: 1, expected: 0
[ 3.651223][ T33] CPU: 0 PID: 33 Comm: kworker/u8:2 Tainted: G W 6.9.0-rc5-00037-g831d3c6cc6f0 #1
[ 3.651979][ T33] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
[ 3.651979][ T33] Workqueue: async async_run_entry_fn
[ 3.651979][ T33] Call Trace:
[ 3.651979][ T33] <TASK>
[ 3.651979][ T33] dump_stack_lvl (lib/dump_stack.c:116)
[ 3.651979][ T33] __might_resched (kernel/sched/core.c:10198)
[ 3.651979][ T33] kmem_cache_alloc (include/linux/kernel.h:73 include/linux/sched/mm.h:315 mm/slub.c:3746 mm/slub.c:3827 mm/slub.c:3852)
[ 3.651979][ T33] alloc_empty_file (fs/file_table.c:203)
[ 3.651979][ T33] path_openat (fs/namei.c:3796)
[ 3.651979][ T33] do_filp_open (fs/namei.c:3833)
[ 3.651979][ T33] file_open_name (fs/open.c:1352)
[ 3.651979][ T33] filp_open (fs/open.c:1371)
[ 3.651979][ T33] do_name (init/initramfs.c:373)
[ 3.651979][ T33] flush_buffer (init/initramfs.c:452 init/initramfs.c:464)
[ 3.651979][ T33] ? __pfx_flush_buffer (init/initramfs.c:458)
[ 3.651979][ T33] __gunzip (lib/decompress_inflate.c:161)
[ 3.651979][ T33] ? __pfx_nofill (lib/decompress_inflate.c:37)
[ 3.651979][ T33] unpack_to_rootfs (init/initramfs.c:520)
[ 3.651979][ T33] ? __pfx_error (init/initramfs.c:59)
[ 3.651979][ T33] do_populate_rootfs (init/initramfs.c:714)
[ 3.651979][ T33] async_run_entry_fn (kernel/async.c:136)
[ 3.651979][ T33] process_scheduled_works (kernel/workqueue.c:3259)
[ 3.651979][ T33] worker_thread (include/linux/list.h:373 kernel/workqueue.c:955 kernel/workqueue.c:3417)
[ 3.651979][ T33] ? __pfx_worker_thread (kernel/workqueue.c:3362)
[ 3.651979][ T33] kthread (kernel/kthread.c:390)
[ 3.651979][ T33] ? __pfx_kthread (kernel/kthread.c:341)
[ 3.651979][ T33] ret_from_fork (arch/x86/kernel/process.c:153)
[ 3.651979][ T33] ? __pfx_kthread (kernel/kthread.c:341)
[ 3.651979][ T33] ret_from_fork_asm (arch/x86/entry/entry_64.S:257)
[ 3.651979][ T33] </TASK>
[ 3.705833][ T33] Freeing initrd memory: 185612K
The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20240425/202404251525.39b4af4e-lkp@intel.com
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH 2/2] openat2: add OA2_INHERIT_CRED flag
From: stsp @ 2024-04-25 9:23 UTC (permalink / raw)
To: Al Viro
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Andy Lutomirski,
Christian Brauner, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240425023127.GH2118490@ZenIV>
25.04.2024 05:31, Al Viro пишет:
> Incidentally, suppose you have the same process run with stdin opened
> (r/o) by root. F_SETFD it to O_CLOEXEC, then use your open with
> dirfd being 0, pathname - "" and flags - O_RDWR.
I actually checked this with the test-case.
It seems to return ENOENT:
Breakpoint 1, openat2 (dirfd=0, pathname=0x7fffffffdbee "",
how=0x7fffffffd5e0, size=24) at tst.c:13
13 return syscall(SYS_openat2, dirfd, pathname, how, size);
(gdb) fin
Run till exit from #0 openat2 (dirfd=0, pathname=0x7fffffffdbee "",
how=0x7fffffffd5e0, size=24) at tst.c:13
0x000000000040167b in main (argc=3, argv=0x7fffffffd7b8) at tst.c:140
140 fd = openat2(0, efile, &how1, sizeof(how1));
Value returned is $1 = -1
(gdb) list
135 err = fcntl(0, F_SETFD, O_CLOEXEC);
136 if (err) {
137 perror("fcntl(F_SETFD)");
138 return EXIT_FAILURE;
139 }
140 fd = openat2(0, efile, &how1, sizeof(how1));
141 if (fd == -1) {
142 perror("openat2(1)");
143 // return EXIT_FAILURE;
144 } else {
(gdb) p errno
$2 = 2
So it seems the creds can't be stolen
from a non-dir fd, but I wonder why
ENOENT is returned instead of ENOTDIR.
Such ENOENT is not dicumented in a
man page of openat2(), so I guess there
is some problem here even w/o my patch. :)
^ permalink raw reply
* Re: [PATCH v4 0/2] implement OA2_INHERIT_CRED flag for openat2()
From: Christian Brauner @ 2024-04-25 9:54 UTC (permalink / raw)
To: stsp
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <6b46528a-965f-410a-9e6f-9654c5e9dba2@yandex.ru>
On Wed, Apr 24, 2024 at 08:50:30PM +0300, stsp wrote:
> 24.04.2024 19:09, Christian Brauner пишет:
> > This smells ripe enough to serve as an attack vector in non-obvious
> > ways. And in general this has the potential to confuse the hell out
> > unsuspecting userspace.
>
> Unsuspecting user-space will simply
> not use this flag. What do you mean?
>
>
> > They can now suddenly get sent such
> > special-sauce files
>
> There are no any special files.
> This flag helps you to open a file on
> which you currently have no perms
> to open, but had those in the past.
>
>
> > such as this that they have no way of recognizing as
> > there's neither an FMODE_* flag nor is the OA2_* flag recorded so it's
> > not available in F_GETFL.
> >
> > There's not even a way to restrict that new flag because no LSM ever
> > sees it. So that behavior might break LSM assumptions as well.
> >
> > And it is effectively usable to steal credentials. If process A opens a
> > directory with uid/gid 0 then sends that directory fd via AF_UNIX or
> > something to process B then process B can inherit the uid/gid of process
>
> No, it doesn't inherit anything.
> The inheritance happens only for
> a duration of an open() call, helping
> open() to succeed. The creds are
> reverted when open() completed.
>
> The only theoretically possible attack
> would be to open some file you'd never
> intended to open. Also note that a
> very minimal sed of creds is overridden:
> fsuid, fsgid, groupinfo.
>
> > A by specifying OA2_* with no way for process A to prevent this - not
> > even through an LSM.
>
> If process B doesn't use that flag, it
> inherits nothing, no matter what process
> A did or passed via a socket.
> So an unaware process that doesn't
> use that flag, is completely unaffected.
The point is that the original opener has no way to prevent his creds
being abused by a completely unrelated process later on. Something I've
clearly explained in my mail.
>
> > The permission checking model that we have right now is already baroque.
> > I see zero reason to add more complexity for the sake of "lightweight
> > sandboxing". We have LSMs and namespaces for stuff like this.
> >
> > NAK.
>
> I don't think it is fair to say NAK
> without actually reading the patch
> or asking its author for clarifications.
> Even though you didn't ask, I provided
> my clarifications above, as I find that
> a polite action.
I'm not sure what you don't understand or why you need further
clarification. Your patch allows any opener using your new flag to steal
the uid/gid/whatever from the original opener. It was even worse in the
first version where the whole struct cred of the original opener was
used. It's obviously a glaring security hole that's opened up by this.
Let alone that the justification "It's useful for some lightweight
sandboxing" is absolutely not sufficient to justify substantial shifts
in the permission model.
The NAK stands.
^ permalink raw reply
* Re: [PATCH v4 0/2] implement OA2_INHERIT_CRED flag for openat2()
From: stsp @ 2024-04-25 10:12 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Jan Kara, Jeff Layton, Chuck Lever,
Alexander Aring, David Laight, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche
In-Reply-To: <20240425-ausfiel-beabsichtigen-a2ef9126ebda@brauner>
25.04.2024 12:54, Christian Brauner пишет:
> I'm not sure what you don't understand or why you need further
> clarification. Your patch allows any opener using your new flag to steal
> the uid/gid/whatever from the original opener.
No, absolutely impossible (see below).
> It was even worse in the
> first version where the whole struct cred of the original opener was
> used. It's obviously a glaring security hole that's opened up by this.
Well, it was the second version actually
(first one only had fsuid/fsgid), but no,
its the same thing either way.
The creds are overridden for a diration of
an openat2() syscall. It doesn't matter
what uid/gid are there, because they are
not used during openat2(), and are reverted
back at the end. The only reason I decided
to get back to fsuid/fsgid, were the capabilities,
which I don't want to be raised during openat2().
> Let alone that the justification "It's useful for some lightweight
> sandboxing" is absolutely not sufficient to justify substantial shifts
> in the permission model.
>
> The NAK stands.
But I am sure you still don't understand
what exactly the patch does, so why not
to ask instead of asserting?
You say uid/gid can be stolen, but no,
it can't: the creds are reverted. Only
fsuid/fsgid (and caps in v2 of the patch)
actually affect openat2(), but nothing is
"leaked" after openat2() finished.
That said, Viro already pointed to the actual
problem, and the patch-testing bot did the
same. So I have a valid complains already,
and you don't have to add the invalid ones
to them. :)
^ permalink raw reply
* Re: [PATCH v5 2/3] VT: Add KDFONTINFO ioctl
From: Helge Deller @ 2024-04-25 10:33 UTC (permalink / raw)
To: Alexey Gladkov
Cc: Greg Kroah-Hartman, Jiri Slaby, LKML, kbd, linux-api, linux-fbdev,
linux-serial
In-Reply-To: <ZiD50WZZv3OOad7L@example.org>
On 4/18/24 12:45, Alexey Gladkov wrote:
> On Wed, Apr 17, 2024 at 09:31:53PM +0200, Helge Deller wrote:
>> On 4/17/24 19:37, Alexey Gladkov wrote:
>>> Each driver has its own restrictions on font size. There is currently no
>>> way to understand what the requirements are. The new ioctl allows
>>> userspace to get the minimum and maximum font size values.
>>>
>>> Acked-by: Helge Deller <deller@gmx.de>
>>> Signed-off-by: Alexey Gladkov <legion@kernel.org>
>>> ---
>>> drivers/tty/vt/vt.c | 24 ++++++++++++++++++++++++
>>> drivers/tty/vt/vt_ioctl.c | 13 +++++++++++++
>>> include/linux/console.h | 3 +++
>>> include/linux/vt_kern.h | 1 +
>>> include/uapi/linux/kd.h | 14 ++++++++++++++
>>> 5 files changed, 55 insertions(+)
>>>
>>> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
>>> index 9b5b98dfc8b4..e8db0e9ea674 100644
>>> --- a/drivers/tty/vt/vt.c
>>> +++ b/drivers/tty/vt/vt.c
>>> @@ -4851,6 +4851,30 @@ int con_font_op(struct vc_data *vc, struct console_font_op *op)
>>> return -ENOSYS;
>>> }
>>>
>>> +int con_font_info(struct vc_data *vc, struct console_font_info *info)
>>> +{
>>> + int rc;
>>> +
>>> + info->min_height = 0;
>>> + info->max_height = max_font_height;
>>> +
>>> + info->min_width = 0;
>>> + info->max_width = max_font_width;
>>> +
>>> + info->flags = KD_FONT_INFO_FLAG_LOW_SIZE | KD_FONT_INFO_FLAG_HIGH_SIZE;
>>> +
>>> + console_lock();
>>> + if (vc->vc_mode != KD_TEXT)
>>> + rc = -EINVAL;
>>> + else if (vc->vc_sw->con_font_info)
>>> + rc = vc->vc_sw->con_font_info(vc, info);
>>> + else
>>> + rc = -ENOSYS;
>>> + console_unlock();
>>> +
>>> + return rc;
>>> +}
>>> +
>>> /*
>>> * Interface exported to selection and vcs.
>>> */
>>> diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c
>>> index 4b91072f3a4e..9a2f8081f650 100644
>>> --- a/drivers/tty/vt/vt_ioctl.c
>>> +++ b/drivers/tty/vt/vt_ioctl.c
>>> @@ -479,6 +479,19 @@ static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd,
>>> break;
>>> }
>>>
>>> + case KDFONTINFO: {
>>> + struct console_font_info fnt_info;
>>> +
>>> + memset(&fnt_info, 0, sizeof(fnt_info));
>>> +
>>> + ret = con_font_info(vc, &fnt_info);
>>> + if (ret)
>>> + return ret;
>>> + if (copy_to_user(up, &fnt_info, sizeof(fnt_info)))
>>> + return -EFAULT;
>>> + break;
>>> + }
>>> +
>>> default:
>>> return -ENOIOCTLCMD;
>>> }
>>> diff --git a/include/linux/console.h b/include/linux/console.h
>>> index 31a8f5b85f5d..4b798322aa01 100644
>>> --- a/include/linux/console.h
>>> +++ b/include/linux/console.h
>>> @@ -21,6 +21,7 @@
>>> #include <linux/vesa.h>
>>>
>>> struct vc_data;
>>> +struct console_font_info;
>>> struct console_font_op;
>>> struct console_font;
>>> struct module;
>>> @@ -102,6 +103,8 @@ struct consw {
>>> bool (*con_switch)(struct vc_data *vc);
>>> bool (*con_blank)(struct vc_data *vc, enum vesa_blank_mode blank,
>>> bool mode_switch);
>>> + int (*con_font_info)(struct vc_data *vc,
>>> + struct console_font_info *info);
>>> int (*con_font_set)(struct vc_data *vc,
>>> const struct console_font *font,
>>> unsigned int vpitch, unsigned int flags);
>>> diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
>>> index d008c3d0a9bb..383b3a4f6113 100644
>>> --- a/include/linux/vt_kern.h
>>> +++ b/include/linux/vt_kern.h
>>> @@ -33,6 +33,7 @@ void do_blank_screen(int entering_gfx);
>>> void do_unblank_screen(int leaving_gfx);
>>> void poke_blanked_console(void);
>>> int con_font_op(struct vc_data *vc, struct console_font_op *op);
>>> +int con_font_info(struct vc_data *vc, struct console_font_info *info);
>>> int con_set_cmap(unsigned char __user *cmap);
>>> int con_get_cmap(unsigned char __user *cmap);
>>> void scrollback(struct vc_data *vc);
>>> diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
>>> index 8ddb2219a84b..68b715ad4d5c 100644
>>> --- a/include/uapi/linux/kd.h
>>> +++ b/include/uapi/linux/kd.h
>>> @@ -185,6 +185,20 @@ struct console_font {
>>>
>>> #define KD_FONT_FLAG_DONT_RECALC 1 /* Don't recalculate hw charcell size [compat] */
>>>
>>> +/* font information */
>>> +
>>> +#define KD_FONT_INFO_FLAG_LOW_SIZE _BITUL(0) /* 256 */
>>> +#define KD_FONT_INFO_FLAG_HIGH_SIZE _BITUL(1) /* 512 */
>>
>> Do we really need those bits?
>> You set a default min/max font size in con_font_info() above,
>> and all drivers can override those values.
>> So, there are always min/max sizes available.
>
> These bits are not about the minimum and maximum glyph size, but about the
> number of glyphs in the font.
>
> Maybe this is an overkill, but sticon has this check:
>
> if ((w < 6) || (h < 6) || (w > 32) || (h > 32) || (vpitch != 32)
> || (op->charcount != 256 && op->charcount != 512))
>
> [ to be honest, I don’t know why this driver doesn’t accept a glyph of
> width 4 ]
I think there was no technical limitation when I added that.
It's just that the font would be so small...
> I thought it would be worth fixing the maximum number of requirements in
> the drivers since I started adding a new ioctl.
Ok.
Helge
^ permalink raw reply
* Re: [PATCH v2 0/2] implement OA2_INHERIT_CRED flag for openat2()
From: stsp @ 2024-04-25 11:02 UTC (permalink / raw)
To: Andy Lutomirski
Cc: linux-kernel, Stefan Metzmacher, Eric Biederman, Alexander Viro,
Andy Lutomirski, Christian Brauner, Jan Kara, Jeff Layton,
Chuck Lever, Alexander Aring, linux-fsdevel, linux-api,
Paolo Bonzini, Christian Göttsche, Aleksa Sarai
In-Reply-To: <CALCETrWswr5jAzD9BkdCqLX=d8vReO8O9dVmZfL7HXdvwkft9g@mail.gmail.com>
25.04.2024 03:43, Andy Lutomirski пишет:
> But you missed the FMODE_CRED part!
OK, I thought its not needed if fd
is limited to the one created by the
same process. But your explanation
is quite clear on that its needed anyway,
or otherwise the unsuspecting process
doesn't fully drop his privs.
Thank you for explaining that bit.
Which leaves just one question: is
such an opt-in enough or not?
Viro points it may not be enough,
but doesn't explain why exactly.
Maybe we need such an opt-in, and
it should be dropped on exec() and
on passing via unix fd? I don't know
what additional restrictions are needed,
as Viro didn't clarify that part, but the
opt-in is needed for sure.
^ permalink raw reply
* Re: [PATCH v5 2/3] VT: Add KDFONTINFO ioctl
From: Alexey Gladkov @ 2024-04-25 11:06 UTC (permalink / raw)
To: Helge Deller
Cc: Greg Kroah-Hartman, Jiri Slaby, LKML, kbd, linux-api, linux-fbdev,
linux-serial
In-Reply-To: <9993ad2d-48a8-43be-ae41-4b8d710c1ea7@gmx.de>
On Thu, Apr 25, 2024 at 12:33:28PM +0200, Helge Deller wrote:
> >>> diff --git a/include/uapi/linux/kd.h b/include/uapi/linux/kd.h
> >>> index 8ddb2219a84b..68b715ad4d5c 100644
> >>> --- a/include/uapi/linux/kd.h
> >>> +++ b/include/uapi/linux/kd.h
> >>> @@ -185,6 +185,20 @@ struct console_font {
> >>>
> >>> #define KD_FONT_FLAG_DONT_RECALC 1 /* Don't recalculate hw charcell size [compat] */
> >>>
> >>> +/* font information */
> >>> +
> >>> +#define KD_FONT_INFO_FLAG_LOW_SIZE _BITUL(0) /* 256 */
> >>> +#define KD_FONT_INFO_FLAG_HIGH_SIZE _BITUL(1) /* 512 */
> >>
> >> Do we really need those bits?
> >> You set a default min/max font size in con_font_info() above,
> >> and all drivers can override those values.
> >> So, there are always min/max sizes available.
> >
> > These bits are not about the minimum and maximum glyph size, but about the
> > number of glyphs in the font.
> >
> > Maybe this is an overkill, but sticon has this check:
> >
> > if ((w < 6) || (h < 6) || (w > 32) || (h > 32) || (vpitch != 32)
> > || (op->charcount != 256 && op->charcount != 512))
> >
> > [ to be honest, I don’t know why this driver doesn’t accept a glyph of
> > width 4 ]
>
> I think there was no technical limitation when I added that.
> It's just that the font would be so small...
If so, then I can remove min_height/min_width from the ioctl structure.
And most likely the flags can also be left empty since at the moment all
drivers support 512.
> > I thought it would be worth fixing the maximum number of requirements in
> > the drivers since I started adding a new ioctl.
>
> Ok.
>
> Helge
>
--
Rgrds, legion
^ permalink raw reply
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Jonathan Cameron @ 2024-04-25 11:33 UTC (permalink / raw)
To: Dan Williams
Cc: linux-cxl, Sreenivas Bagalkote, Brett Henning, Harold Johnson,
Sumanesh Samanta, linux-kernel, Davidlohr Bueso, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, linuxarm, linux-api,
Lorenzo Pieralisi, Natu, Mahesh
In-Reply-To: <66284d5e8ac01_690a29431@dwillia2-xfh.jf.intel.com.notmuch>
On Tue, 23 Apr 2024 17:07:58 -0700
Dan Williams <dan.j.williams@intel.com> wrote:
> Jonathan Cameron wrote:
> [..]
> > > It is not clear to me that this material makes sense to house in
> > > drivers/ vs tools/ or even out-of-tree just for maintenance burden
> > > relief of keeping the universes separated. What does the Linux kernel
> > > project get out of carrying this in mainline alongside the inband code?
> >
> > I'm not sure what you mean by in band. Aim here was to discuss
> > in-band drivers for switch CCI etc. Same reason from a kernel point of
> > view for why we include embedded drivers. I'll interpret in band
> > as host driven and not inband as FM-API stuff.
> >
> > > I do think the mailbox refactoring to support non-CXL use cases is
> > > interesting, but only so far as refactoring is consumed for inband use
> > > cases like RAS API.
> >
> > If I read this right, I disagree with the 'only so far' bit.
> >
> > In all substantial ways we should support BMC use case of the Linux Kernel
> > at a similar level to how we support forms of Linux Distros.
>
> I think we need to talk in terms of specifics, because in the general
> case I do not see the blockage. OpenBMC currently is based on v6.6.28
> and carries 136 patches. An additional patch to turn off raw commands
> restrictions over there would not even be noticed.
Hi Dan,
That I'm fine with - it's a reasonable middle ground where we ensure
they have a sensible upstream solution, but just patch around the
taint etc in the downstream projects.
Note 136 patches is tiny for a distro and reflects their hard work
upstreaming stuff.
>
> > It may not be our target market as developers for particular parts of
> > our companies, but we should not block those who want to support it.
>
> It is also the case that there is a responsibility to build maintainable
> kernel interfaces that can be reasoned about, especially with devices as
> powerful as CXL that are trusted to host system memory and be caching
> agents. For example, I do not want to be in the position of auditing
> whether proposed tunnels and passthroughs violate lockdown expectations.
I agree with that - this can be made dependent on not locking down
in the same way lots of other somewhat dangerous interfaces are.
We can relax that restriction as things do get audited - sure
tunnels aren't going to be on that allowed list in the short term.
>
> Also, the assertion that these kernels will be built with
> CONFIG_SECURITY_LOCKDOWN_LSM=n and likely CONFIG_STRICT_DEVMEM=n, then
> the entire user-mode driver ABI is available for use. CXL commands are
> simple polled mmio, does Linux really benefit from carrying drivers in
> the kernel that the kernel itself does not care about?
Sure we could it in userspace... It's bad engineering, limits the design
to polling only and uses a bunch of interfaces we put a lot of effort into
telling people not to use except for debug.
I really don't see the advantage in pushing a project/group of projects
all of which are picking the upstream kernel up directly, to do a dirty
hack. We loose all the advantages of a proper well maintained kernel
driver purely on the argument that one use model is not the same as
this one. Sensible security lockdown requirements is fine (along
with all the other kernel features that must be disable for that
to work), making open kernel development on for a large Linux
market harder is not.
>
> [..]
> > Switch CCI Driver: PCI driver doing everything beyond the CXL mbox specific bit.
> > Type 3 Stack: All the normal stack just with the CXL Mailbox specific stuff factored
> > out. Note we can move different amounts of shared logic in here, but
> > in essence it deals with the extra layer on top of the raw MMPT mbox.
> > MMPT Mbox: Mailbox as per the PCI spec.
> > RAS API: Shared RAS API specific infrastructure used by other drivers.
>
> Once the CXL mailbox core is turned into a library for kernel internal
> consumers, like RAS API, or CXL accelerators, then it becomes easier to
> add a Switch CCI consumer (perhaps as an out-of-tree module in tools/),
> but it is still not clear why the kernel benefits from that arrangement.
We can argue later on this. But from my point of view, in tree module not
in tools is a must. Doesn't have to be in drivers/cxl if its simply the
association aspect that is a blocker.
>
> This is less about blocking developers that have different goals it is
> about finding the right projects / places to solve the problem
> especially when disjoint design goals are in play and user space drivers
> might be in reach.
Key here is this is not a case of openBMC is the one true distro on
which all Linux BMCs and fabric management platforms are based.
So we are really talking a random out of tree driver with all the maintenance
overhead that brings. Yuck.
So I don't see there being any good solution out side of upstream support
other than pushing this to be a userspace hack.
>
> [..]
> > > > The various CXL upstream developers and maintainers may have
> > > > differing views of course, but my current understanding is we want
> > > > to support 1 and 2, but are very resistant to 3!
> > >
> > > 1, yes, 2, need to see the patches, and agree on 3.
> >
> > If we end up with top architecture of the diagrams above, 2 will look pretty
> > similar to last version of the switch-cci patches. So raw commands only + taint.
> > Factoring out MMPT is another layer that doesn't make that much difference in
> > practice to this discussion. Good to have, but the reuse here would be one layer
> > above that.
> >
> > Or we just say go for second proposed architecture and 0 impact on the
> > CXL specific code, just reuse of the MMPT layer. I'd imagine people will get
> > grumpy on code duplication (and we'll spend years rejecting patch sets that
> > try to share the cdoe) but there should be no maintenance burden as
> > a result.
>
> I am assuming that the shared code between MMPT and CXL will happen and
> that all of the command infrastructure is where centralized policy can
> not keep up.
There is actually very little to MMPT, but sure there will be some sharing
of code and the policy won't sit in that shared part as it is protocol
specific.
> If OpenBMC wants to land a driver that consumes the MMPT
> core in tools/ that would seem to satisfy both the concerns of mainline
> not shipping ABI that host kernels need to strictly reason about while
> letting OpenBMC not need to carry out-of-tree patches indefinitely.
We can argue that detail later, but tools is not in my opinion
a valid solution to supporting properly maintained upstream drivers.
Its a hack for test and example modules only. The path of just
blocking this driver in any locked down situation seems much more inline
with kernel norms. It is also extremely bad precedence.
Jonathan
p.s. I don't care in the slightest about openBMC (other than general
warm fuzzy feelings about a good open source project), I do care
rather more about BMCs and other fabric managers.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox