* [PATCH v6 1/2] platform/x86: asus_wmi: Support throttle thermal policy
From: Leonid Maksymchuk @ 2019-12-15 14:26 UTC (permalink / raw)
To: linux-kernel
Cc: platform-driver-x86, linux-api, acpi4asus-user, chiu,
yurii.pavlovskyi, kristian, andy, dvhart, corentin.chary,
Leonid Maksymchuk
In-Reply-To: <20191215142527.13780-1-leonmaxx@gmail.com>
Throttle thermal policy ACPI device is used to control CPU cooling and
throttling. This patch adds sysfs entry for setting current mode and
Fn+F5 hotkey that switches to next.
Policy modes:
* 0x00 - default
* 0x01 - overboost
* 0x02 - silent
Signed-off-by: Leonid Maksymchuk <leonmaxx@gmail.com>
---
.../ABI/testing/sysfs-platform-asus-wmi | 10 ++
drivers/platform/x86/asus-wmi.c | 113 ++++++++++++++++++
include/linux/platform_data/x86/asus-wmi.h | 1 +
3 files changed, 124 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
index 9e99f2909612..1efac0ddb417 100644
--- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
+++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
@@ -46,3 +46,13 @@ Description:
* 0 - normal,
* 1 - overboost,
* 2 - silent
+
+What: /sys/devices/platform/<platform>/throttle_thermal_policy
+Date: Dec 2019
+KernelVersion: 5.6
+Contact: "Leonid Maksymchuk" <leonmaxx@gmail.com>
+Description:
+ Throttle thermal policy mode:
+ * 0 - default,
+ * 1 - overboost,
+ * 2 - silent
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 821b08e01635..f10ec9d745e5 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -61,6 +61,7 @@ MODULE_LICENSE("GPL");
#define NOTIFY_KBD_BRTDWN 0xc5
#define NOTIFY_KBD_BRTTOGGLE 0xc7
#define NOTIFY_KBD_FBM 0x99
+#define NOTIFY_KBD_TTP 0xae
#define ASUS_WMI_FNLOCK_BIOS_DISABLED BIT(0)
@@ -81,6 +82,10 @@ MODULE_LICENSE("GPL");
#define ASUS_FAN_BOOST_MODE_SILENT_MASK 0x02
#define ASUS_FAN_BOOST_MODES_MASK 0x03
+#define ASUS_THROTTLE_THERMAL_POLICY_DEFAULT 0
+#define ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST 1
+#define ASUS_THROTTLE_THERMAL_POLICY_SILENT 2
+
#define USB_INTEL_XUSB2PR 0xD0
#define PCI_DEVICE_ID_INTEL_LYNXPOINT_LP_XHCI 0x9c31
@@ -198,6 +203,9 @@ struct asus_wmi {
u8 fan_boost_mode_mask;
u8 fan_boost_mode;
+ bool throttle_thermal_policy_available;
+ u8 throttle_thermal_policy_mode;
+
// The RSOC controls the maximum charging percentage.
bool battery_rsoc_available;
@@ -1724,6 +1732,98 @@ static ssize_t fan_boost_mode_store(struct device *dev,
// Fan boost mode: 0 - normal, 1 - overboost, 2 - silent
static DEVICE_ATTR_RW(fan_boost_mode);
+/* Throttle thermal policy ****************************************************/
+
+static int throttle_thermal_policy_check_present(struct asus_wmi *asus)
+{
+ u32 result;
+ int err;
+
+ asus->throttle_thermal_policy_available = false;
+
+ err = asus_wmi_get_devstate(asus,
+ ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY,
+ &result);
+ if (err) {
+ if (err == -ENODEV)
+ return 0;
+ return err;
+ }
+
+ if (result & ASUS_WMI_DSTS_PRESENCE_BIT)
+ asus->throttle_thermal_policy_available = true;
+
+ return 0;
+}
+
+static int throttle_thermal_policy_write(struct asus_wmi *asus)
+{
+ int err;
+ u8 value;
+ u32 retval;
+
+ value = asus->throttle_thermal_policy_mode;
+
+ err = asus_wmi_set_devstate(ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY,
+ value, &retval);
+ if (err) {
+ pr_warn("Failed to set throttle thermal policy: %d\n", err);
+ return err;
+ }
+
+ if (retval != 1) {
+ pr_warn("Failed to set throttle thermal policy (retval): 0x%x\n",
+ retval);
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
+{
+ u8 new_mode = asus->throttle_thermal_policy_mode + 1;
+
+ if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
+ new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
+
+ asus->throttle_thermal_policy_mode = new_mode;
+ return throttle_thermal_policy_write(asus);
+}
+
+static ssize_t throttle_thermal_policy_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct asus_wmi *asus = dev_get_drvdata(dev);
+ u8 mode = asus->throttle_thermal_policy_mode;
+
+ return scnprintf(buf, PAGE_SIZE, "%d\n", mode);
+}
+
+static ssize_t throttle_thermal_policy_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int result;
+ u8 new_mode;
+ struct asus_wmi *asus = dev_get_drvdata(dev);
+
+ result = kstrtou8(buf, 10, &new_mode);
+ if (result < 0)
+ return result;
+
+ if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
+ return -EINVAL;
+
+ asus->throttle_thermal_policy_mode = new_mode;
+ throttle_thermal_policy_write(asus);
+
+ return count;
+}
+
+// Throttle thermal policy: 0 - default, 1 - overboost, 2 - silent
+static DEVICE_ATTR_RW(throttle_thermal_policy);
+
/* Backlight ******************************************************************/
static int read_backlight_power(struct asus_wmi *asus)
@@ -2005,6 +2105,11 @@ static void asus_wmi_handle_event_code(int code, struct asus_wmi *asus)
return;
}
+ if (asus->throttle_thermal_policy_available && code == NOTIFY_KBD_TTP) {
+ throttle_thermal_policy_switch_next(asus);
+ return;
+ }
+
if (is_display_toggle(code) && asus->driver->quirks->no_display_toggle)
return;
@@ -2155,6 +2260,7 @@ static struct attribute *platform_attributes[] = {
&dev_attr_lid_resume.attr,
&dev_attr_als_enable.attr,
&dev_attr_fan_boost_mode.attr,
+ &dev_attr_throttle_thermal_policy.attr,
NULL
};
@@ -2178,6 +2284,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
devid = ASUS_WMI_DEVID_ALS_ENABLE;
else if (attr == &dev_attr_fan_boost_mode.attr)
ok = asus->fan_boost_mode_available;
+ else if (attr == &dev_attr_throttle_thermal_policy.attr)
+ ok = asus->throttle_thermal_policy_available;
if (devid != -1)
ok = !(asus_wmi_get_devstate_simple(asus, devid) < 0);
@@ -2437,6 +2545,10 @@ static int asus_wmi_add(struct platform_device *pdev)
if (err)
goto fail_fan_boost_mode;
+ err = throttle_thermal_policy_check_present(asus);
+ if (err)
+ goto fail_throttle_thermal_policy;
+
err = asus_wmi_sysfs_init(asus->platform_device);
if (err)
goto fail_sysfs;
@@ -2521,6 +2633,7 @@ static int asus_wmi_add(struct platform_device *pdev)
fail_input:
asus_wmi_sysfs_exit(asus->platform_device);
fail_sysfs:
+fail_throttle_thermal_policy:
fail_fan_boost_mode:
fail_platform:
kfree(asus);
diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
index 60249e22e844..d39fc658c320 100644
--- a/include/linux/platform_data/x86/asus-wmi.h
+++ b/include/linux/platform_data/x86/asus-wmi.h
@@ -58,6 +58,7 @@
#define ASUS_WMI_DEVID_LIGHT_SENSOR 0x00050022 /* ?? */
#define ASUS_WMI_DEVID_LIGHTBAR 0x00050025
#define ASUS_WMI_DEVID_FAN_BOOST_MODE 0x00110018
+#define ASUS_WMI_DEVID_THROTTLE_THERMAL_POLICY 0x00120075
/* Misc */
#define ASUS_WMI_DEVID_CAMERA 0x00060013
--
2.24.0
^ permalink raw reply related
* [PATCH v6 2/2] platform/x86: asus_wmi: Set throttle thermal policy to default
From: Leonid Maksymchuk @ 2019-12-15 14:27 UTC (permalink / raw)
To: linux-kernel
Cc: platform-driver-x86, linux-api, acpi4asus-user, chiu,
yurii.pavlovskyi, kristian, andy, dvhart, corentin.chary,
Leonid Maksymchuk
In-Reply-To: <20191215142527.13780-1-leonmaxx@gmail.com>
ASUS TUF FX705DY/FX505DY starts in silent mode and under heavy
CPU load it overheats and drops CPU frequency to 399MHz and stays
at it until reboot [1]. Set throttle thermal policy to default
to avoid overheating and throttlig.
[1] Link: https://bugzilla.kernel.org/show_bug.cgi?id=203733
Signed-off-by: Leonid Maksymchuk <leonmaxx@gmail.com>
---
drivers/platform/x86/asus-wmi.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index f10ec9d745e5..469f1a852719 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -1780,6 +1780,15 @@ static int throttle_thermal_policy_write(struct asus_wmi *asus)
return 0;
}
+static int throttle_thermal_policy_set_default(struct asus_wmi *asus)
+{
+ if (!asus->throttle_thermal_policy_available)
+ return 0;
+
+ asus->throttle_thermal_policy_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
+ return throttle_thermal_policy_write(asus);
+}
+
static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
{
u8 new_mode = asus->throttle_thermal_policy_mode + 1;
@@ -2548,6 +2557,8 @@ static int asus_wmi_add(struct platform_device *pdev)
err = throttle_thermal_policy_check_present(asus);
if (err)
goto fail_throttle_thermal_policy;
+ else
+ throttle_thermal_policy_set_default(asus);
err = asus_wmi_sysfs_init(asus->platform_device);
if (err)
--
2.24.0
^ permalink raw reply related
* Re: [PATCH] openat2: switch to __attribute__((packed)) for open_how
From: Florian Weimer @ 2019-12-15 19:48 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Alexander Viro, Jeff Layton, J. Bruce Fields, Shuah Khan, dev,
containers, linux-api, linux-fsdevel, linux-kernel,
linux-kselftest
In-Reply-To: <20191213222351.14071-1-cyphar@cyphar.com>
* Aleksa Sarai:
> diff --git a/tools/testing/selftests/openat2/helpers.h b/tools/testing/selftests/openat2/helpers.h
> index 43ca5ceab6e3..eb1535c8fa2e 100644
> --- a/tools/testing/selftests/openat2/helpers.h
> +++ b/tools/testing/selftests/openat2/helpers.h
> @@ -32,17 +32,16 @@
> * O_TMPFILE} are set.
> *
> * @flags: O_* flags.
> - * @mode: O_CREAT/O_TMPFILE file mode.
> * @resolve: RESOLVE_* flags.
> + * @mode: O_CREAT/O_TMPFILE file mode.
> */
> struct open_how {
> - __aligned_u64 flags;
> + __u64 flags;
> + __u64 resolve;
> __u16 mode;
> - __u16 __padding[3]; /* must be zeroed */
> - __aligned_u64 resolve;
> -};
> +} __attribute__((packed));
>
> -#define OPEN_HOW_SIZE_VER0 24 /* sizeof first published struct */
> +#define OPEN_HOW_SIZE_VER0 18 /* sizeof first published struct */
> #define OPEN_HOW_SIZE_LATEST OPEN_HOW_SIZE_VER0
A userspace ABI that depends on GCC extensions probably isn't a good
idea. Even with GCC, it will not work well with some future
extensions because it pretty much rules out having arrays or other
members that are access through pointers. Current GCC does not carry
over the packed-ness of the struct to addresses of its members.
^ permalink raw reply
* Re: [PATCH] openat2: switch to __attribute__((packed)) for open_how
From: Aleksa Sarai @ 2019-12-15 20:55 UTC (permalink / raw)
To: Florian Weimer
Cc: Alexander Viro, Jeff Layton, J. Bruce Fields, Shuah Khan, dev,
containers, linux-api, linux-fsdevel, linux-kernel,
linux-kselftest
In-Reply-To: <87o8w9bcaf.fsf@mid.deneb.enyo.de>
[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]
On 2019-12-15, Florian Weimer <fw@deneb.enyo.de> wrote:
> * Aleksa Sarai:
>
> > diff --git a/tools/testing/selftests/openat2/helpers.h b/tools/testing/selftests/openat2/helpers.h
> > index 43ca5ceab6e3..eb1535c8fa2e 100644
> > --- a/tools/testing/selftests/openat2/helpers.h
> > +++ b/tools/testing/selftests/openat2/helpers.h
> > @@ -32,17 +32,16 @@
> > * O_TMPFILE} are set.
> > *
> > * @flags: O_* flags.
> > - * @mode: O_CREAT/O_TMPFILE file mode.
> > * @resolve: RESOLVE_* flags.
> > + * @mode: O_CREAT/O_TMPFILE file mode.
> > */
> > struct open_how {
> > - __aligned_u64 flags;
> > + __u64 flags;
> > + __u64 resolve;
> > __u16 mode;
> > - __u16 __padding[3]; /* must be zeroed */
> > - __aligned_u64 resolve;
> > -};
> > +} __attribute__((packed));
> >
> > -#define OPEN_HOW_SIZE_VER0 24 /* sizeof first published struct */
> > +#define OPEN_HOW_SIZE_VER0 18 /* sizeof first published struct */
> > #define OPEN_HOW_SIZE_LATEST OPEN_HOW_SIZE_VER0
>
> A userspace ABI that depends on GCC extensions probably isn't a good
> idea. Even with GCC, it will not work well with some future
> extensions because it pretty much rules out having arrays or other
> members that are access through pointers. Current GCC does not carry
> over the packed-ness of the struct to addresses of its members.
Right, those are also good points.
Okay, I'm going to send a separate patch which changes the return value
for invalid __padding to -E2BIG, and moves the padding to the end of the
struct (along with open_how.mode). That should fix all of the warts I
raised, without running into the numerous problems with
__attribute__((packed)) of which I am now aware.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v6 0/2] add performance reporting support to FPGA DFL drivers
From: Xu Yilum @ 2019-12-16 1:01 UTC (permalink / raw)
To: Will Deacon
Cc: Wu Hao, mdf, mark.rutland, linux-fpga, linux-kernel, linux-api,
atull, gregkh
In-Reply-To: <20191209024527.GA22625@hao-dev>
On Mon, Dec 09, 2019 at 10:45:27AM +0800, Wu Hao wrote:
> On Mon, Nov 25, 2019 at 04:08:39PM +0800, Wu Hao wrote:
> > On Mon, Nov 25, 2019 at 08:01:28AM +0000, Will Deacon wrote:
> > > On Mon, Nov 25, 2019 at 11:34:12AM +0800, Wu Hao wrote:
> > > > Hi Will and Mark,
> > > >
> > > > Could you please help us on review this patchset? as this patchset mainly
> > > > introduced a new perf driver following the similar way as drivers/perf/*.
> > >
> > > Why is it not under drivers/perf/, then?
> >
> > Hi Will
> >
> > Thanks for the quick response. This is one sub feature for DFL based FPGAs,
> > and we plan to put this sub feature together with others, including related
> > documentation. It only registers a standard perf pmu for its userspace
> > interfaces.
> >
> > >
> > > > This patchset has been submitted for a long time but didn't receive any
> > > > comment after v4. we appreciate any review comments! thanks in advance. :)
> > >
> > > Hmm, not sure I saw the previous versions. Guessing I wasn't on cc?
> >
> > We switched to perf API from v4, and started ccing you and Mark from v5. :)
>
> Hi Will
>
> Did you get a chance to look into this patchset?
>
> Thanks
> Hao
Hi Will
Did you have time to look into this patchset? We have done review work
for FPGA part. And as a perf driver, we appreciate your comments.
Thanks
Yilun
>
> >
> > Thanks
> > Hao
> >
> > >
> > > Will
^ permalink raw reply
* [PATCH v8 0/1] ns: introduce binfmt_misc namespace
From: Laurent Vivier @ 2019-12-16 9:12 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kurz, Jann Horn, Andrei Vagin, linux-api, Dmitry Safonov,
James Bottomley, Jan Kiszka, Christian Brauner, linux-fsdevel,
containers, Alexander Viro, Eric Biederman, Henning Schild,
Cédric Le Goater, Laurent Vivier
v8: s/file->f_path.dentry/file_dentry(file)/
v7: Use the new mount API
Replace
static struct dentry *bm_mount(struct file_system_type *fs_type,
int flags, const char *dev_name, void *data)
{
struct user_namespace *ns = current_user_ns();
return mount_ns(fs_type, flags, data, ns, ns,
bm_fill_super);
}
by
static void bm_free(struct fs_context *fc)
{
if (fc->s_fs_info)
put_user_ns(fc->s_fs_info);
}
static int bm_get_tree(struct fs_context *fc)
{
return get_tree_keyed(fc, bm_fill_super, get_user_ns(fc->user_ns));
}
static const struct fs_context_operations bm_context_ops = {
.free = bm_free,
.get_tree = bm_get_tree,
};
static int bm_init_fs_context(struct fs_context *fc)
{
fc->ops = &bm_context_ops;
return 0;
}
v6: Return &init_binfmt_ns instead of NULL in binfmt_ns()
This should never happen, but to stay safe return a
value we can use.
change subject from "RFC" to "PATCH"
v5: Use READ_ONCE()/WRITE_ONCE()
move mount pointer struct init to bm_fill_super() and add smp_wmb()
remove useless NULL value init
add WARN_ON_ONCE()
v4: first user namespace is initialized with &init_binfmt_ns,
all new user namespaces are initialized with a NULL and use
the one of the first parent that is not NULL. The pointer
is initialized to a valid value the first time the binfmt_misc
fs is mounted in the current user namespace.
This allows to not change the way it was working before:
new ns inherits values from its parent, and if parent value is modified
(or parent creates its own binfmt entry by mounting the fs) child
inherits it (unless it has itself mounted the fs).
v3: create a structure to store binfmt_misc data,
add a pointer to this structure in the user_namespace structure,
in init_user_ns structure this pointer points to an init_binfmt_ns
structure. And all new user namespaces point to this init structure.
A new binfmt namespace structure is allocated if the binfmt_misc
filesystem is mounted in a user namespace that is not the initial
one but its binfmt namespace pointer points to the initial one.
add override_creds()/revert_creds() around open_exec() in
bm_register_write()
v2: no new namespace, binfmt_misc data are now part of
the mount namespace
I put this in mount namespace instead of user namespace
because the mount namespace is already needed and
I don't want to force to have the user namespace for that.
As this is a filesystem, it seems logic to have it here.
This allows to define a new interpreter for each new container.
But the main goal is to be able to chroot to a directory
using a binfmt_misc interpreter without being root.
I have a modified version of unshare at:
https://github.com/vivier/util-linux.git branch unshare-chroot
with some new options to unshare binfmt_misc namespace and to chroot
to a directory.
If you have a directory /chroot/powerpc/jessie containing debian for powerpc
binaries and a qemu-ppc interpreter, you can do for instance:
$ uname -a
Linux fedora28-wor-2 4.19.0-rc5+ #18 SMP Mon Oct 1 00:32:34 CEST 2018 x86_64 x86_64 x86_64 GNU/Linux
$ ./unshare --map-root-user --fork --pid \
--load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff:/qemu-ppc:OC" \
--root=/chroot/powerpc/jessie /bin/bash -l
# uname -a
Linux fedora28-wor-2 4.19.0-rc5+ #18 SMP Mon Oct 1 00:32:34 CEST 2018 ppc GNU/Linux
# id
uid=0(root) gid=0(root) groups=0(root),65534(nogroup)
# ls -l
total 5940
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:58 bin
drwxr-xr-x. 2 nobody nogroup 4096 Jun 17 20:26 boot
drwxr-xr-x. 4 nobody nogroup 4096 Aug 12 00:08 dev
drwxr-xr-x. 42 nobody nogroup 4096 Sep 28 07:25 etc
drwxr-xr-x. 3 nobody nogroup 4096 Sep 28 07:25 home
drwxr-xr-x. 9 nobody nogroup 4096 Aug 12 00:58 lib
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 media
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 mnt
drwxr-xr-x. 3 nobody nogroup 4096 Aug 12 13:09 opt
dr-xr-xr-x. 143 nobody nogroup 0 Sep 30 23:02 proc
-rwxr-xr-x. 1 nobody nogroup 6009712 Sep 28 07:22 qemu-ppc
drwx------. 3 nobody nogroup 4096 Aug 12 12:54 root
drwxr-xr-x. 3 nobody nogroup 4096 Aug 12 00:08 run
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:58 sbin
drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 srv
drwxr-xr-x. 2 nobody nogroup 4096 Apr 6 2015 sys
drwxrwxrwt. 2 nobody nogroup 4096 Sep 28 10:31 tmp
drwxr-xr-x. 10 nobody nogroup 4096 Aug 12 00:08 usr
drwxr-xr-x. 11 nobody nogroup 4096 Aug 12 00:08 var
If you want to use the qemu binary provided by your distro, you can use
--load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff:/bin/qemu-ppc-static:OCF"
With the 'F' flag, qemu-ppc-static will be then loaded from the main root
filesystem before switching to the chroot.
Another example is to use the 'P' flag in one chroot and not in another one (useful in a test
environment to test different configurations of the same interpreter):
./unshare --fork --pid --mount-proc --map-root-user --load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff://usr/bin/qemu-ppc-noargv0:OCF" --root=/chroot/powerpc/jessie /bin/bash -l
root@localhost:/# sh -c 'echo $0'
/bin/sh
./unshare --fork --pid --mount-proc --map-root-user --load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff://usr/bin/qemu-ppc-argv0:OCFP" --root=/chroot/powerpc/jessie /bin/bash -l
root@localhost:/# sh -c 'echo $0'
sh
Laurent Vivier (1):
ns: add binfmt_misc to the user namespace
fs/binfmt_misc.c | 115 +++++++++++++++++++++++++--------
include/linux/user_namespace.h | 15 +++++
kernel/user.c | 14 ++++
kernel/user_namespace.c | 3 +
4 files changed, 119 insertions(+), 28 deletions(-)
--
2.23.0
^ permalink raw reply
* [PATCH v8 1/1] ns: add binfmt_misc to the user namespace
From: Laurent Vivier @ 2019-12-16 9:12 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kurz, Jann Horn, Andrei Vagin, linux-api, Dmitry Safonov,
James Bottomley, Jan Kiszka, Christian Brauner, linux-fsdevel,
containers, Alexander Viro, Eric Biederman, Henning Schild,
Cédric Le Goater, Laurent Vivier
In-Reply-To: <20191216091220.465626-1-laurent@vivier.eu>
This patch allows to have a different binfmt_misc configuration
for each new user namespace. By default, the binfmt_misc configuration
is the one of the previous level, but if the binfmt_misc filesystem is
mounted in the new namespace a new empty binfmt instance is created and
used in this namespace.
For instance, using "unshare" we can start a chroot of another
architecture and configure the binfmt_misc interpreter without being root
to run the binaries in this chroot.
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
Acked-by: Andrei Vagin <avagin@gmail.com>
Tested-by: Henning Schild <henning.schild@siemens.com>
---
fs/binfmt_misc.c | 115 +++++++++++++++++++++++++--------
include/linux/user_namespace.h | 15 +++++
kernel/user.c | 14 ++++
kernel/user_namespace.c | 3 +
4 files changed, 119 insertions(+), 28 deletions(-)
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index cdb45829354d..17fa1f56ca2e 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -40,9 +40,6 @@ enum {
VERBOSE_STATUS = 1 /* make it zero to save 400 bytes kernel memory */
};
-static LIST_HEAD(entries);
-static int enabled = 1;
-
enum {Enabled, Magic};
#define MISC_FMT_PRESERVE_ARGV0 (1 << 31)
#define MISC_FMT_OPEN_BINARY (1 << 30)
@@ -62,10 +59,7 @@ typedef struct {
struct file *interp_file;
} Node;
-static DEFINE_RWLOCK(entries_lock);
static struct file_system_type bm_fs_type;
-static struct vfsmount *bm_mnt;
-static int entry_count;
/*
* Max length of the register string. Determined by:
@@ -82,18 +76,37 @@ static int entry_count;
*/
#define MAX_REGISTER_LENGTH 1920
+static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns)
+{
+ struct binfmt_namespace *b_ns;
+
+ while (ns) {
+ b_ns = READ_ONCE(ns->binfmt_ns);
+ if (b_ns)
+ return b_ns;
+ ns = ns->parent;
+ }
+ /* as the first user namespace is initialized with
+ * &init_binfmt_ns we should never come here
+ * but we try to stay safe by logging a warning
+ * and returning a sane value
+ */
+ WARN_ON_ONCE(1);
+ return &init_binfmt_ns;
+}
+
/*
* Check if we support the binfmt
* if we do, return the node, else NULL
* locking is done in load_misc_binary
*/
-static Node *check_file(struct linux_binprm *bprm)
+static Node *check_file(struct binfmt_namespace *ns, struct linux_binprm *bprm)
{
char *p = strrchr(bprm->interp, '.');
struct list_head *l;
/* Walk all the registered handlers. */
- list_for_each(l, &entries) {
+ list_for_each(l, &ns->entries) {
Node *e = list_entry(l, Node, list);
char *s;
int j;
@@ -135,17 +148,18 @@ static int load_misc_binary(struct linux_binprm *bprm)
struct file *interp_file = NULL;
int retval;
int fd_binary = -1;
+ struct binfmt_namespace *ns = binfmt_ns(current_user_ns());
retval = -ENOEXEC;
- if (!enabled)
+ if (!ns->enabled)
return retval;
/* to keep locking time low, we copy the interpreter string */
- read_lock(&entries_lock);
- fmt = check_file(bprm);
+ read_lock(&ns->entries_lock);
+ fmt = check_file(ns, bprm);
if (fmt)
dget(fmt->dentry);
- read_unlock(&entries_lock);
+ read_unlock(&ns->entries_lock);
if (!fmt)
return retval;
@@ -611,19 +625,19 @@ static void bm_evict_inode(struct inode *inode)
kfree(e);
}
-static void kill_node(Node *e)
+static void kill_node(struct binfmt_namespace *ns, Node *e)
{
struct dentry *dentry;
- write_lock(&entries_lock);
+ write_lock(&ns->entries_lock);
list_del_init(&e->list);
- write_unlock(&entries_lock);
+ write_unlock(&ns->entries_lock);
dentry = e->dentry;
drop_nlink(d_inode(dentry));
d_drop(dentry);
dput(dentry);
- simple_release_fs(&bm_mnt, &entry_count);
+ simple_release_fs(&ns->bm_mnt, &ns->entry_count);
}
/* /<entry> */
@@ -653,6 +667,9 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
struct dentry *root;
Node *e = file_inode(file)->i_private;
int res = parse_command(buffer, count);
+ struct binfmt_namespace *ns;
+
+ ns = binfmt_ns(file_dentry(file)->d_sb->s_user_ns);
switch (res) {
case 1:
@@ -669,7 +686,7 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
inode_lock(d_inode(root));
if (!list_empty(&e->list))
- kill_node(e);
+ kill_node(ns, e);
inode_unlock(d_inode(root));
break;
@@ -695,6 +712,7 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
struct inode *inode;
struct super_block *sb = file_inode(file)->i_sb;
struct dentry *root = sb->s_root, *dentry;
+ struct binfmt_namespace *ns;
int err = 0;
e = create_entry(buffer, count);
@@ -718,7 +736,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
if (!inode)
goto out2;
- err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count);
+ ns = binfmt_ns(file_dentry(file)->d_sb->s_user_ns);
+ err = simple_pin_fs(&bm_fs_type, &ns->bm_mnt,
+ &ns->entry_count);
if (err) {
iput(inode);
inode = NULL;
@@ -727,12 +747,16 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
if (e->flags & MISC_FMT_OPEN_FILE) {
struct file *f;
+ const struct cred *old_cred;
+ old_cred = override_creds(file->f_cred);
f = open_exec(e->interpreter);
+ revert_creds(old_cred);
if (IS_ERR(f)) {
err = PTR_ERR(f);
pr_notice("register: failed to install interpreter file %s\n", e->interpreter);
- simple_release_fs(&bm_mnt, &entry_count);
+ simple_release_fs(&ns->bm_mnt,
+ &ns->entry_count);
iput(inode);
inode = NULL;
goto out2;
@@ -745,9 +769,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
inode->i_fop = &bm_entry_operations;
d_instantiate(dentry, inode);
- write_lock(&entries_lock);
- list_add(&e->list, &entries);
- write_unlock(&entries_lock);
+ write_lock(&ns->entries_lock);
+ list_add(&e->list, &ns->entries);
+ write_unlock(&ns->entries_lock);
err = 0;
out2:
@@ -772,7 +796,9 @@ static const struct file_operations bm_register_operations = {
static ssize_t
bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
{
- char *s = enabled ? "enabled\n" : "disabled\n";
+ struct binfmt_namespace *ns =
+ binfmt_ns(file_dentry(file)->d_sb->s_user_ns);
+ char *s = ns->enabled ? "enabled\n" : "disabled\n";
return simple_read_from_buffer(buf, nbytes, ppos, s, strlen(s));
}
@@ -780,25 +806,28 @@ bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
static ssize_t bm_status_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
+ struct binfmt_namespace *ns;
int res = parse_command(buffer, count);
struct dentry *root;
+ ns = binfmt_ns(file_dentry(file)->d_sb->s_user_ns);
switch (res) {
case 1:
/* Disable all handlers. */
- enabled = 0;
+ ns->enabled = 0;
break;
case 2:
/* Enable all handlers. */
- enabled = 1;
+ ns->enabled = 1;
break;
case 3:
/* Delete all handlers. */
root = file_inode(file)->i_sb->s_root;
inode_lock(d_inode(root));
- while (!list_empty(&entries))
- kill_node(list_first_entry(&entries, Node, list));
+ while (!list_empty(&ns->entries))
+ kill_node(ns, list_first_entry(&ns->entries,
+ Node, list));
inode_unlock(d_inode(root));
break;
@@ -825,24 +854,53 @@ static const struct super_operations s_ops = {
static int bm_fill_super(struct super_block *sb, struct fs_context *fc)
{
int err;
+ struct user_namespace *ns = sb->s_user_ns;
static const struct tree_descr bm_files[] = {
[2] = {"status", &bm_status_operations, S_IWUSR|S_IRUGO},
[3] = {"register", &bm_register_operations, S_IWUSR},
/* last one */ {""}
};
+ /* create a new binfmt namespace
+ * if we are not in the first user namespace
+ * but the binfmt namespace is the first one
+ */
+ if (READ_ONCE(ns->binfmt_ns) == NULL) {
+ struct binfmt_namespace *new_ns;
+
+ new_ns = kmalloc(sizeof(struct binfmt_namespace),
+ GFP_KERNEL);
+ if (new_ns == NULL)
+ return -ENOMEM;
+ INIT_LIST_HEAD(&new_ns->entries);
+ new_ns->enabled = 1;
+ rwlock_init(&new_ns->entries_lock);
+ new_ns->bm_mnt = NULL;
+ new_ns->entry_count = 0;
+ /* ensure new_ns is completely initialized before sharing it */
+ smp_wmb();
+ WRITE_ONCE(ns->binfmt_ns, new_ns);
+ }
+
err = simple_fill_super(sb, BINFMTFS_MAGIC, bm_files);
if (!err)
sb->s_op = &s_ops;
return err;
}
+static void bm_free(struct fs_context *fc)
+{
+ if (fc->s_fs_info)
+ put_user_ns(fc->s_fs_info);
+}
+
static int bm_get_tree(struct fs_context *fc)
{
- return get_tree_single(fc, bm_fill_super);
+ return get_tree_keyed(fc, bm_fill_super, get_user_ns(fc->user_ns));
}
static const struct fs_context_operations bm_context_ops = {
+ .free = bm_free,
.get_tree = bm_get_tree,
};
@@ -861,6 +919,7 @@ static struct file_system_type bm_fs_type = {
.owner = THIS_MODULE,
.name = "binfmt_misc",
.init_fs_context = bm_init_fs_context,
+ .fs_flags = FS_USERNS_MOUNT,
.kill_sb = kill_litter_super,
};
MODULE_ALIAS_FS("binfmt_misc");
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index fb9f4f799554..16e6f3a97a01 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -52,6 +52,18 @@ enum ucount_type {
UCOUNT_COUNTS,
};
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+struct binfmt_namespace {
+ struct list_head entries;
+ rwlock_t entries_lock;
+ int enabled;
+ struct vfsmount *bm_mnt;
+ int entry_count;
+} __randomize_layout;
+
+extern struct binfmt_namespace init_binfmt_ns;
+#endif
+
struct user_namespace {
struct uid_gid_map uid_map;
struct uid_gid_map gid_map;
@@ -86,6 +98,9 @@ struct user_namespace {
#endif
struct ucounts *ucounts;
int ucount_max[UCOUNT_COUNTS];
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+ struct binfmt_namespace *binfmt_ns;
+#endif
} __randomize_layout;
struct ucounts {
diff --git a/kernel/user.c b/kernel/user.c
index 5235d7f49982..092b2b4d47a6 100644
--- a/kernel/user.c
+++ b/kernel/user.c
@@ -20,6 +20,17 @@
#include <linux/user_namespace.h>
#include <linux/proc_ns.h>
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+struct binfmt_namespace init_binfmt_ns = {
+ .entries = LIST_HEAD_INIT(init_binfmt_ns.entries),
+ .enabled = 1,
+ .entries_lock = __RW_LOCK_UNLOCKED(init_binfmt_ns.entries_lock),
+ .bm_mnt = NULL,
+ .entry_count = 0,
+};
+EXPORT_SYMBOL_GPL(init_binfmt_ns);
+#endif
+
/*
* userns count is 1 for root user, 1 for init_uts_ns,
* and 1 for... ?
@@ -67,6 +78,9 @@ struct user_namespace init_user_ns = {
.keyring_name_list = LIST_HEAD_INIT(init_user_ns.keyring_name_list),
.keyring_sem = __RWSEM_INITIALIZER(init_user_ns.keyring_sem),
#endif
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+ .binfmt_ns = &init_binfmt_ns,
+#endif
};
EXPORT_SYMBOL_GPL(init_user_ns);
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 8eadadc478f9..f42c32269e20 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -191,6 +191,9 @@ static void free_user_ns(struct work_struct *work)
kfree(ns->projid_map.forward);
kfree(ns->projid_map.reverse);
}
+#if IS_ENABLED(CONFIG_BINFMT_MISC)
+ kfree(ns->binfmt_ns);
+#endif
retire_userns_sysctls(ns);
key_free_user_ns(ns);
ns_free_inum(&ns->ns);
--
2.23.0
^ permalink raw reply related
* Re: [PATCH v7 1/1] ns: add binfmt_misc to the user namespace
From: Laurent Vivier @ 2019-12-16 9:13 UTC (permalink / raw)
To: mtk.manpages
Cc: James Bottomley, Henning Schild, lkml, Dmitry Safonov,
linux-fsdevel@vger.kernel.org, Eric Biederman, Linux API,
Andrei Vagin, Cédric Le Goater, Greg Kurz, Jann Horn,
Containers, Alexander Viro, Jan Kiszka
In-Reply-To: <CAKgNAkiaKJZMA0pzvwDa75CxfULTL1LmOZDzhW0Y5TmL7nBGZw@mail.gmail.com>
Le 14/12/2019 à 13:32, Michael Kerrisk (man-pages) a écrit :
> Hello Laurent,
>
> On Sat, 14 Dec 2019 at 12:35, Laurent Vivier <laurent@vivier.eu> wrote:
>>
>> Le 13/12/2019 à 20:59, James Bottomley a écrit :
>>> On Fri, 2019-12-13 at 18:51 +0100, Henning Schild wrote:
>>>> Hi all,
>>>>
>>>> that is a very useful contribution, which will hopefully be
>>>> considered.
>>>
>>> I'm technically the maintainer on the you touched it last you own it
>>> basis, so if Christian's concerns get addressed I'll shepherd it
>>> upstream.
>>
>> Thank you.
>>
>> I update this in the next days and re-send the patch.
>
> Would you also be so kind as to craft a patch for the
> user_namespaces(7) manual page describing the changes (sent to me,
> linux-man@vger.kernel.org, and the other parties already in CC)?
>
> If you do not have the time to familiarize yourself with groff/man
> markup, a patch that uses plain text is fine; I can handle the
> formatting.
I will send a patch for the user_namespaces(7) manual.
Thanks,
Laurent
^ permalink raw reply
* Re: [PATCH v8 0/1] ns: introduce binfmt_misc namespace
From: Christian Brauner @ 2019-12-16 9:46 UTC (permalink / raw)
To: Laurent Vivier
Cc: linux-kernel, Greg Kurz, Jann Horn, Andrei Vagin, linux-api,
Dmitry Safonov, James Bottomley, Jan Kiszka, linux-fsdevel,
containers, Alexander Viro, Eric Biederman, Henning Schild,
Cédric Le Goater, keescook
In-Reply-To: <20191216091220.465626-1-laurent@vivier.eu>
On Mon, Dec 16, 2019 at 10:12:19AM +0100, Laurent Vivier wrote:
> v8: s/file->f_path.dentry/file_dentry(file)/
>
> v7: Use the new mount API
>
> Replace
>
> static struct dentry *bm_mount(struct file_system_type *fs_type,
> int flags, const char *dev_name, void *data)
> {
> struct user_namespace *ns = current_user_ns();
>
> return mount_ns(fs_type, flags, data, ns, ns,
> bm_fill_super);
> }
>
> by
>
> static void bm_free(struct fs_context *fc)
> {
> if (fc->s_fs_info)
> put_user_ns(fc->s_fs_info);
> }
>
> static int bm_get_tree(struct fs_context *fc)
> {
> return get_tree_keyed(fc, bm_fill_super, get_user_ns(fc->user_ns));
> }
>
> static const struct fs_context_operations bm_context_ops = {
> .free = bm_free,
> .get_tree = bm_get_tree,
> };
>
> static int bm_init_fs_context(struct fs_context *fc)
> {
> fc->ops = &bm_context_ops;
> return 0;
> }
>
> v6: Return &init_binfmt_ns instead of NULL in binfmt_ns()
> This should never happen, but to stay safe return a
> value we can use.
> change subject from "RFC" to "PATCH"
>
> v5: Use READ_ONCE()/WRITE_ONCE()
> move mount pointer struct init to bm_fill_super() and add smp_wmb()
> remove useless NULL value init
> add WARN_ON_ONCE()
>
> v4: first user namespace is initialized with &init_binfmt_ns,
> all new user namespaces are initialized with a NULL and use
> the one of the first parent that is not NULL. The pointer
> is initialized to a valid value the first time the binfmt_misc
> fs is mounted in the current user namespace.
> This allows to not change the way it was working before:
> new ns inherits values from its parent, and if parent value is modified
> (or parent creates its own binfmt entry by mounting the fs) child
> inherits it (unless it has itself mounted the fs).
>
> v3: create a structure to store binfmt_misc data,
> add a pointer to this structure in the user_namespace structure,
> in init_user_ns structure this pointer points to an init_binfmt_ns
> structure. And all new user namespaces point to this init structure.
> A new binfmt namespace structure is allocated if the binfmt_misc
> filesystem is mounted in a user namespace that is not the initial
> one but its binfmt namespace pointer points to the initial one.
> add override_creds()/revert_creds() around open_exec() in
> bm_register_write()
>
> v2: no new namespace, binfmt_misc data are now part of
> the mount namespace
> I put this in mount namespace instead of user namespace
> because the mount namespace is already needed and
> I don't want to force to have the user namespace for that.
> As this is a filesystem, it seems logic to have it here.
>
> This allows to define a new interpreter for each new container.
>
> But the main goal is to be able to chroot to a directory
> using a binfmt_misc interpreter without being root.
>
> I have a modified version of unshare at:
>
> https://github.com/vivier/util-linux.git branch unshare-chroot
>
> with some new options to unshare binfmt_misc namespace and to chroot
> to a directory.
>
> If you have a directory /chroot/powerpc/jessie containing debian for powerpc
> binaries and a qemu-ppc interpreter, you can do for instance:
>
> $ uname -a
> Linux fedora28-wor-2 4.19.0-rc5+ #18 SMP Mon Oct 1 00:32:34 CEST 2018 x86_64 x86_64 x86_64 GNU/Linux
> $ ./unshare --map-root-user --fork --pid \
> --load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff:/qemu-ppc:OC" \
> --root=/chroot/powerpc/jessie /bin/bash -l
> # uname -a
> Linux fedora28-wor-2 4.19.0-rc5+ #18 SMP Mon Oct 1 00:32:34 CEST 2018 ppc GNU/Linux
> # id
> uid=0(root) gid=0(root) groups=0(root),65534(nogroup)
> # ls -l
> total 5940
> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:58 bin
> drwxr-xr-x. 2 nobody nogroup 4096 Jun 17 20:26 boot
> drwxr-xr-x. 4 nobody nogroup 4096 Aug 12 00:08 dev
> drwxr-xr-x. 42 nobody nogroup 4096 Sep 28 07:25 etc
> drwxr-xr-x. 3 nobody nogroup 4096 Sep 28 07:25 home
> drwxr-xr-x. 9 nobody nogroup 4096 Aug 12 00:58 lib
> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 media
> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 mnt
> drwxr-xr-x. 3 nobody nogroup 4096 Aug 12 13:09 opt
> dr-xr-xr-x. 143 nobody nogroup 0 Sep 30 23:02 proc
> -rwxr-xr-x. 1 nobody nogroup 6009712 Sep 28 07:22 qemu-ppc
> drwx------. 3 nobody nogroup 4096 Aug 12 12:54 root
> drwxr-xr-x. 3 nobody nogroup 4096 Aug 12 00:08 run
> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:58 sbin
> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 srv
> drwxr-xr-x. 2 nobody nogroup 4096 Apr 6 2015 sys
> drwxrwxrwt. 2 nobody nogroup 4096 Sep 28 10:31 tmp
> drwxr-xr-x. 10 nobody nogroup 4096 Aug 12 00:08 usr
> drwxr-xr-x. 11 nobody nogroup 4096 Aug 12 00:08 var
>
> If you want to use the qemu binary provided by your distro, you can use
>
> --load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff:/bin/qemu-ppc-static:OCF"
>
> With the 'F' flag, qemu-ppc-static will be then loaded from the main root
> filesystem before switching to the chroot.
>
> Another example is to use the 'P' flag in one chroot and not in another one (useful in a test
> environment to test different configurations of the same interpreter):
>
> ./unshare --fork --pid --mount-proc --map-root-user --load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff://usr/bin/qemu-ppc-noargv0:OCF" --root=/chroot/powerpc/jessie /bin/bash -l
> root@localhost:/# sh -c 'echo $0'
> /bin/sh
>
> ./unshare --fork --pid --mount-proc --map-root-user --load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff://usr/bin/qemu-ppc-argv0:OCFP" --root=/chroot/powerpc/jessie /bin/bash -l
> root@localhost:/# sh -c 'echo $0'
> sh
Hey Laurent,
We have quite some time before the v5.6 merge window opens. So I would
really like for this new feature to come with proper testing!
I know you've been waiting on this patch quite some time but adding
tests will not endanger v5.6 inclusion - lest we find significant bugs. ;)
Christian
^ permalink raw reply
* Re: [PATCH v8 0/1] ns: introduce binfmt_misc namespace
From: Laurent Vivier @ 2019-12-16 9:53 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel, Greg Kurz, Jann Horn, Andrei Vagin, linux-api,
Dmitry Safonov, James Bottomley, Jan Kiszka, linux-fsdevel,
containers, Alexander Viro, Eric Biederman, Henning Schild,
Cédric Le Goater, keescook
In-Reply-To: <20191216094615.xlhxoze3umjn2tzy@wittgenstein>
Le 16/12/2019 à 10:46, Christian Brauner a écrit :
> On Mon, Dec 16, 2019 at 10:12:19AM +0100, Laurent Vivier wrote:
>> v8: s/file->f_path.dentry/file_dentry(file)/
>>
>> v7: Use the new mount API
>>
>> Replace
>>
>> static struct dentry *bm_mount(struct file_system_type *fs_type,
>> int flags, const char *dev_name, void *data)
>> {
>> struct user_namespace *ns = current_user_ns();
>>
>> return mount_ns(fs_type, flags, data, ns, ns,
>> bm_fill_super);
>> }
>>
>> by
>>
>> static void bm_free(struct fs_context *fc)
>> {
>> if (fc->s_fs_info)
>> put_user_ns(fc->s_fs_info);
>> }
>>
>> static int bm_get_tree(struct fs_context *fc)
>> {
>> return get_tree_keyed(fc, bm_fill_super, get_user_ns(fc->user_ns));
>> }
>>
>> static const struct fs_context_operations bm_context_ops = {
>> .free = bm_free,
>> .get_tree = bm_get_tree,
>> };
>>
>> static int bm_init_fs_context(struct fs_context *fc)
>> {
>> fc->ops = &bm_context_ops;
>> return 0;
>> }
>>
>> v6: Return &init_binfmt_ns instead of NULL in binfmt_ns()
>> This should never happen, but to stay safe return a
>> value we can use.
>> change subject from "RFC" to "PATCH"
>>
>> v5: Use READ_ONCE()/WRITE_ONCE()
>> move mount pointer struct init to bm_fill_super() and add smp_wmb()
>> remove useless NULL value init
>> add WARN_ON_ONCE()
>>
>> v4: first user namespace is initialized with &init_binfmt_ns,
>> all new user namespaces are initialized with a NULL and use
>> the one of the first parent that is not NULL. The pointer
>> is initialized to a valid value the first time the binfmt_misc
>> fs is mounted in the current user namespace.
>> This allows to not change the way it was working before:
>> new ns inherits values from its parent, and if parent value is modified
>> (or parent creates its own binfmt entry by mounting the fs) child
>> inherits it (unless it has itself mounted the fs).
>>
>> v3: create a structure to store binfmt_misc data,
>> add a pointer to this structure in the user_namespace structure,
>> in init_user_ns structure this pointer points to an init_binfmt_ns
>> structure. And all new user namespaces point to this init structure.
>> A new binfmt namespace structure is allocated if the binfmt_misc
>> filesystem is mounted in a user namespace that is not the initial
>> one but its binfmt namespace pointer points to the initial one.
>> add override_creds()/revert_creds() around open_exec() in
>> bm_register_write()
>>
>> v2: no new namespace, binfmt_misc data are now part of
>> the mount namespace
>> I put this in mount namespace instead of user namespace
>> because the mount namespace is already needed and
>> I don't want to force to have the user namespace for that.
>> As this is a filesystem, it seems logic to have it here.
>>
>> This allows to define a new interpreter for each new container.
>>
>> But the main goal is to be able to chroot to a directory
>> using a binfmt_misc interpreter without being root.
>>
>> I have a modified version of unshare at:
>>
>> https://github.com/vivier/util-linux.git branch unshare-chroot
>>
>> with some new options to unshare binfmt_misc namespace and to chroot
>> to a directory.
>>
>> If you have a directory /chroot/powerpc/jessie containing debian for powerpc
>> binaries and a qemu-ppc interpreter, you can do for instance:
>>
>> $ uname -a
>> Linux fedora28-wor-2 4.19.0-rc5+ #18 SMP Mon Oct 1 00:32:34 CEST 2018 x86_64 x86_64 x86_64 GNU/Linux
>> $ ./unshare --map-root-user --fork --pid \
>> --load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff:/qemu-ppc:OC" \
>> --root=/chroot/powerpc/jessie /bin/bash -l
>> # uname -a
>> Linux fedora28-wor-2 4.19.0-rc5+ #18 SMP Mon Oct 1 00:32:34 CEST 2018 ppc GNU/Linux
>> # id
>> uid=0(root) gid=0(root) groups=0(root),65534(nogroup)
>> # ls -l
>> total 5940
>> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:58 bin
>> drwxr-xr-x. 2 nobody nogroup 4096 Jun 17 20:26 boot
>> drwxr-xr-x. 4 nobody nogroup 4096 Aug 12 00:08 dev
>> drwxr-xr-x. 42 nobody nogroup 4096 Sep 28 07:25 etc
>> drwxr-xr-x. 3 nobody nogroup 4096 Sep 28 07:25 home
>> drwxr-xr-x. 9 nobody nogroup 4096 Aug 12 00:58 lib
>> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 media
>> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 mnt
>> drwxr-xr-x. 3 nobody nogroup 4096 Aug 12 13:09 opt
>> dr-xr-xr-x. 143 nobody nogroup 0 Sep 30 23:02 proc
>> -rwxr-xr-x. 1 nobody nogroup 6009712 Sep 28 07:22 qemu-ppc
>> drwx------. 3 nobody nogroup 4096 Aug 12 12:54 root
>> drwxr-xr-x. 3 nobody nogroup 4096 Aug 12 00:08 run
>> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:58 sbin
>> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 srv
>> drwxr-xr-x. 2 nobody nogroup 4096 Apr 6 2015 sys
>> drwxrwxrwt. 2 nobody nogroup 4096 Sep 28 10:31 tmp
>> drwxr-xr-x. 10 nobody nogroup 4096 Aug 12 00:08 usr
>> drwxr-xr-x. 11 nobody nogroup 4096 Aug 12 00:08 var
>>
>> If you want to use the qemu binary provided by your distro, you can use
>>
>> --load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff:/bin/qemu-ppc-static:OCF"
>>
>> With the 'F' flag, qemu-ppc-static will be then loaded from the main root
>> filesystem before switching to the chroot.
>>
>> Another example is to use the 'P' flag in one chroot and not in another one (useful in a test
>> environment to test different configurations of the same interpreter):
>>
>> ./unshare --fork --pid --mount-proc --map-root-user --load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff://usr/bin/qemu-ppc-noargv0:OCF" --root=/chroot/powerpc/jessie /bin/bash -l
>> root@localhost:/# sh -c 'echo $0'
>> /bin/sh
>>
>> ./unshare --fork --pid --mount-proc --map-root-user --load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff://usr/bin/qemu-ppc-argv0:OCFP" --root=/chroot/powerpc/jessie /bin/bash -l
>> root@localhost:/# sh -c 'echo $0'
>> sh
>
> Hey Laurent,
>
> We have quite some time before the v5.6 merge window opens. So I would
> really like for this new feature to come with proper testing!
Are there some already existing tests for binfmt_misc or namespace I can
update to test the new feature?
Thanks,
Laurent
^ permalink raw reply
* Re: [PATCH v8 0/1] ns: introduce binfmt_misc namespace
From: Christian Brauner @ 2019-12-16 10:06 UTC (permalink / raw)
To: Laurent Vivier
Cc: linux-kernel, Greg Kurz, Jann Horn, Andrei Vagin, linux-api,
Dmitry Safonov, James Bottomley, Jan Kiszka, linux-fsdevel,
containers, Alexander Viro, Eric Biederman, Henning Schild,
Cédric Le Goater, keescook
In-Reply-To: <4225d0e8-a907-941f-69ae-c2a9150e6a98@vivier.eu>
On Mon, Dec 16, 2019 at 10:53:28AM +0100, Laurent Vivier wrote:
> Le 16/12/2019 à 10:46, Christian Brauner a écrit :
> > On Mon, Dec 16, 2019 at 10:12:19AM +0100, Laurent Vivier wrote:
> >> v8: s/file->f_path.dentry/file_dentry(file)/
> >>
> >> v7: Use the new mount API
> >>
> >> Replace
> >>
> >> static struct dentry *bm_mount(struct file_system_type *fs_type,
> >> int flags, const char *dev_name, void *data)
> >> {
> >> struct user_namespace *ns = current_user_ns();
> >>
> >> return mount_ns(fs_type, flags, data, ns, ns,
> >> bm_fill_super);
> >> }
> >>
> >> by
> >>
> >> static void bm_free(struct fs_context *fc)
> >> {
> >> if (fc->s_fs_info)
> >> put_user_ns(fc->s_fs_info);
> >> }
> >>
> >> static int bm_get_tree(struct fs_context *fc)
> >> {
> >> return get_tree_keyed(fc, bm_fill_super, get_user_ns(fc->user_ns));
> >> }
> >>
> >> static const struct fs_context_operations bm_context_ops = {
> >> .free = bm_free,
> >> .get_tree = bm_get_tree,
> >> };
> >>
> >> static int bm_init_fs_context(struct fs_context *fc)
> >> {
> >> fc->ops = &bm_context_ops;
> >> return 0;
> >> }
> >>
> >> v6: Return &init_binfmt_ns instead of NULL in binfmt_ns()
> >> This should never happen, but to stay safe return a
> >> value we can use.
> >> change subject from "RFC" to "PATCH"
> >>
> >> v5: Use READ_ONCE()/WRITE_ONCE()
> >> move mount pointer struct init to bm_fill_super() and add smp_wmb()
> >> remove useless NULL value init
> >> add WARN_ON_ONCE()
> >>
> >> v4: first user namespace is initialized with &init_binfmt_ns,
> >> all new user namespaces are initialized with a NULL and use
> >> the one of the first parent that is not NULL. The pointer
> >> is initialized to a valid value the first time the binfmt_misc
> >> fs is mounted in the current user namespace.
> >> This allows to not change the way it was working before:
> >> new ns inherits values from its parent, and if parent value is modified
> >> (or parent creates its own binfmt entry by mounting the fs) child
> >> inherits it (unless it has itself mounted the fs).
> >>
> >> v3: create a structure to store binfmt_misc data,
> >> add a pointer to this structure in the user_namespace structure,
> >> in init_user_ns structure this pointer points to an init_binfmt_ns
> >> structure. And all new user namespaces point to this init structure.
> >> A new binfmt namespace structure is allocated if the binfmt_misc
> >> filesystem is mounted in a user namespace that is not the initial
> >> one but its binfmt namespace pointer points to the initial one.
> >> add override_creds()/revert_creds() around open_exec() in
> >> bm_register_write()
> >>
> >> v2: no new namespace, binfmt_misc data are now part of
> >> the mount namespace
> >> I put this in mount namespace instead of user namespace
> >> because the mount namespace is already needed and
> >> I don't want to force to have the user namespace for that.
> >> As this is a filesystem, it seems logic to have it here.
> >>
> >> This allows to define a new interpreter for each new container.
> >>
> >> But the main goal is to be able to chroot to a directory
> >> using a binfmt_misc interpreter without being root.
> >>
> >> I have a modified version of unshare at:
> >>
> >> https://github.com/vivier/util-linux.git branch unshare-chroot
> >>
> >> with some new options to unshare binfmt_misc namespace and to chroot
> >> to a directory.
> >>
> >> If you have a directory /chroot/powerpc/jessie containing debian for powerpc
> >> binaries and a qemu-ppc interpreter, you can do for instance:
> >>
> >> $ uname -a
> >> Linux fedora28-wor-2 4.19.0-rc5+ #18 SMP Mon Oct 1 00:32:34 CEST 2018 x86_64 x86_64 x86_64 GNU/Linux
> >> $ ./unshare --map-root-user --fork --pid \
> >> --load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff:/qemu-ppc:OC" \
> >> --root=/chroot/powerpc/jessie /bin/bash -l
> >> # uname -a
> >> Linux fedora28-wor-2 4.19.0-rc5+ #18 SMP Mon Oct 1 00:32:34 CEST 2018 ppc GNU/Linux
> >> # id
> >> uid=0(root) gid=0(root) groups=0(root),65534(nogroup)
> >> # ls -l
> >> total 5940
> >> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:58 bin
> >> drwxr-xr-x. 2 nobody nogroup 4096 Jun 17 20:26 boot
> >> drwxr-xr-x. 4 nobody nogroup 4096 Aug 12 00:08 dev
> >> drwxr-xr-x. 42 nobody nogroup 4096 Sep 28 07:25 etc
> >> drwxr-xr-x. 3 nobody nogroup 4096 Sep 28 07:25 home
> >> drwxr-xr-x. 9 nobody nogroup 4096 Aug 12 00:58 lib
> >> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 media
> >> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 mnt
> >> drwxr-xr-x. 3 nobody nogroup 4096 Aug 12 13:09 opt
> >> dr-xr-xr-x. 143 nobody nogroup 0 Sep 30 23:02 proc
> >> -rwxr-xr-x. 1 nobody nogroup 6009712 Sep 28 07:22 qemu-ppc
> >> drwx------. 3 nobody nogroup 4096 Aug 12 12:54 root
> >> drwxr-xr-x. 3 nobody nogroup 4096 Aug 12 00:08 run
> >> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:58 sbin
> >> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 srv
> >> drwxr-xr-x. 2 nobody nogroup 4096 Apr 6 2015 sys
> >> drwxrwxrwt. 2 nobody nogroup 4096 Sep 28 10:31 tmp
> >> drwxr-xr-x. 10 nobody nogroup 4096 Aug 12 00:08 usr
> >> drwxr-xr-x. 11 nobody nogroup 4096 Aug 12 00:08 var
> >>
> >> If you want to use the qemu binary provided by your distro, you can use
> >>
> >> --load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff:/bin/qemu-ppc-static:OCF"
> >>
> >> With the 'F' flag, qemu-ppc-static will be then loaded from the main root
> >> filesystem before switching to the chroot.
> >>
> >> Another example is to use the 'P' flag in one chroot and not in another one (useful in a test
> >> environment to test different configurations of the same interpreter):
> >>
> >> ./unshare --fork --pid --mount-proc --map-root-user --load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff://usr/bin/qemu-ppc-noargv0:OCF" --root=/chroot/powerpc/jessie /bin/bash -l
> >> root@localhost:/# sh -c 'echo $0'
> >> /bin/sh
> >>
> >> ./unshare --fork --pid --mount-proc --map-root-user --load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff://usr/bin/qemu-ppc-argv0:OCFP" --root=/chroot/powerpc/jessie /bin/bash -l
> >> root@localhost:/# sh -c 'echo $0'
> >> sh
> >
> > Hey Laurent,
> >
> > We have quite some time before the v5.6 merge window opens. So I would
> > really like for this new feature to come with proper testing!
>
> Are there some already existing tests for binfmt_misc or namespace I can
> update to test the new feature?
I don't think so but there are tests for other namespace-aware
filesystem. For example, I've added basic tests for binderfs in
tools/testing/selftests/filesystems/binderfs/ and there are some devpts
tests in there (Though the devpts tests don't actually make use of the
kselftest framework so they aren't a great example. I'm not claiming
binderfs is either tbh. :))
You can just place the binfmt_misc tests in there. Helpers for setting
up user namespace and mappings are in there as well. I think you can
just place them in a separate file/header and include it for both
binderfs and binfmt_misc.
I'm happy to review this/answer questions.
Thanks!
Christian
^ permalink raw reply
* Re: [PATCH v8 0/1] ns: introduce binfmt_misc namespace
From: Laurent Vivier @ 2019-12-16 10:08 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel, Greg Kurz, Jann Horn, Andrei Vagin, linux-api,
Dmitry Safonov, James Bottomley, Jan Kiszka, linux-fsdevel,
containers, Alexander Viro, Eric Biederman, Henning Schild,
Cédric Le Goater, keescook
In-Reply-To: <20191216100607.gvbhfqokf3ulkc23@wittgenstein>
Le 16/12/2019 à 11:06, Christian Brauner a écrit :
> On Mon, Dec 16, 2019 at 10:53:28AM +0100, Laurent Vivier wrote:
>> Le 16/12/2019 à 10:46, Christian Brauner a écrit :
>>> On Mon, Dec 16, 2019 at 10:12:19AM +0100, Laurent Vivier wrote:
>>>> v8: s/file->f_path.dentry/file_dentry(file)/
>>>>
>>>> v7: Use the new mount API
>>>>
>>>> Replace
>>>>
>>>> static struct dentry *bm_mount(struct file_system_type *fs_type,
>>>> int flags, const char *dev_name, void *data)
>>>> {
>>>> struct user_namespace *ns = current_user_ns();
>>>>
>>>> return mount_ns(fs_type, flags, data, ns, ns,
>>>> bm_fill_super);
>>>> }
>>>>
>>>> by
>>>>
>>>> static void bm_free(struct fs_context *fc)
>>>> {
>>>> if (fc->s_fs_info)
>>>> put_user_ns(fc->s_fs_info);
>>>> }
>>>>
>>>> static int bm_get_tree(struct fs_context *fc)
>>>> {
>>>> return get_tree_keyed(fc, bm_fill_super, get_user_ns(fc->user_ns));
>>>> }
>>>>
>>>> static const struct fs_context_operations bm_context_ops = {
>>>> .free = bm_free,
>>>> .get_tree = bm_get_tree,
>>>> };
>>>>
>>>> static int bm_init_fs_context(struct fs_context *fc)
>>>> {
>>>> fc->ops = &bm_context_ops;
>>>> return 0;
>>>> }
>>>>
>>>> v6: Return &init_binfmt_ns instead of NULL in binfmt_ns()
>>>> This should never happen, but to stay safe return a
>>>> value we can use.
>>>> change subject from "RFC" to "PATCH"
>>>>
>>>> v5: Use READ_ONCE()/WRITE_ONCE()
>>>> move mount pointer struct init to bm_fill_super() and add smp_wmb()
>>>> remove useless NULL value init
>>>> add WARN_ON_ONCE()
>>>>
>>>> v4: first user namespace is initialized with &init_binfmt_ns,
>>>> all new user namespaces are initialized with a NULL and use
>>>> the one of the first parent that is not NULL. The pointer
>>>> is initialized to a valid value the first time the binfmt_misc
>>>> fs is mounted in the current user namespace.
>>>> This allows to not change the way it was working before:
>>>> new ns inherits values from its parent, and if parent value is modified
>>>> (or parent creates its own binfmt entry by mounting the fs) child
>>>> inherits it (unless it has itself mounted the fs).
>>>>
>>>> v3: create a structure to store binfmt_misc data,
>>>> add a pointer to this structure in the user_namespace structure,
>>>> in init_user_ns structure this pointer points to an init_binfmt_ns
>>>> structure. And all new user namespaces point to this init structure.
>>>> A new binfmt namespace structure is allocated if the binfmt_misc
>>>> filesystem is mounted in a user namespace that is not the initial
>>>> one but its binfmt namespace pointer points to the initial one.
>>>> add override_creds()/revert_creds() around open_exec() in
>>>> bm_register_write()
>>>>
>>>> v2: no new namespace, binfmt_misc data are now part of
>>>> the mount namespace
>>>> I put this in mount namespace instead of user namespace
>>>> because the mount namespace is already needed and
>>>> I don't want to force to have the user namespace for that.
>>>> As this is a filesystem, it seems logic to have it here.
>>>>
>>>> This allows to define a new interpreter for each new container.
>>>>
>>>> But the main goal is to be able to chroot to a directory
>>>> using a binfmt_misc interpreter without being root.
>>>>
>>>> I have a modified version of unshare at:
>>>>
>>>> https://github.com/vivier/util-linux.git branch unshare-chroot
>>>>
>>>> with some new options to unshare binfmt_misc namespace and to chroot
>>>> to a directory.
>>>>
>>>> If you have a directory /chroot/powerpc/jessie containing debian for powerpc
>>>> binaries and a qemu-ppc interpreter, you can do for instance:
>>>>
>>>> $ uname -a
>>>> Linux fedora28-wor-2 4.19.0-rc5+ #18 SMP Mon Oct 1 00:32:34 CEST 2018 x86_64 x86_64 x86_64 GNU/Linux
>>>> $ ./unshare --map-root-user --fork --pid \
>>>> --load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff:/qemu-ppc:OC" \
>>>> --root=/chroot/powerpc/jessie /bin/bash -l
>>>> # uname -a
>>>> Linux fedora28-wor-2 4.19.0-rc5+ #18 SMP Mon Oct 1 00:32:34 CEST 2018 ppc GNU/Linux
>>>> # id
>>>> uid=0(root) gid=0(root) groups=0(root),65534(nogroup)
>>>> # ls -l
>>>> total 5940
>>>> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:58 bin
>>>> drwxr-xr-x. 2 nobody nogroup 4096 Jun 17 20:26 boot
>>>> drwxr-xr-x. 4 nobody nogroup 4096 Aug 12 00:08 dev
>>>> drwxr-xr-x. 42 nobody nogroup 4096 Sep 28 07:25 etc
>>>> drwxr-xr-x. 3 nobody nogroup 4096 Sep 28 07:25 home
>>>> drwxr-xr-x. 9 nobody nogroup 4096 Aug 12 00:58 lib
>>>> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 media
>>>> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 mnt
>>>> drwxr-xr-x. 3 nobody nogroup 4096 Aug 12 13:09 opt
>>>> dr-xr-xr-x. 143 nobody nogroup 0 Sep 30 23:02 proc
>>>> -rwxr-xr-x. 1 nobody nogroup 6009712 Sep 28 07:22 qemu-ppc
>>>> drwx------. 3 nobody nogroup 4096 Aug 12 12:54 root
>>>> drwxr-xr-x. 3 nobody nogroup 4096 Aug 12 00:08 run
>>>> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:58 sbin
>>>> drwxr-xr-x. 2 nobody nogroup 4096 Aug 12 00:08 srv
>>>> drwxr-xr-x. 2 nobody nogroup 4096 Apr 6 2015 sys
>>>> drwxrwxrwt. 2 nobody nogroup 4096 Sep 28 10:31 tmp
>>>> drwxr-xr-x. 10 nobody nogroup 4096 Aug 12 00:08 usr
>>>> drwxr-xr-x. 11 nobody nogroup 4096 Aug 12 00:08 var
>>>>
>>>> If you want to use the qemu binary provided by your distro, you can use
>>>>
>>>> --load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff:/bin/qemu-ppc-static:OCF"
>>>>
>>>> With the 'F' flag, qemu-ppc-static will be then loaded from the main root
>>>> filesystem before switching to the chroot.
>>>>
>>>> Another example is to use the 'P' flag in one chroot and not in another one (useful in a test
>>>> environment to test different configurations of the same interpreter):
>>>>
>>>> ./unshare --fork --pid --mount-proc --map-root-user --load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff://usr/bin/qemu-ppc-noargv0:OCF" --root=/chroot/powerpc/jessie /bin/bash -l
>>>> root@localhost:/# sh -c 'echo $0'
>>>> /bin/sh
>>>>
>>>> ./unshare --fork --pid --mount-proc --map-root-user --load-interp ":qemu-ppc:M::\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x14:\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff://usr/bin/qemu-ppc-argv0:OCFP" --root=/chroot/powerpc/jessie /bin/bash -l
>>>> root@localhost:/# sh -c 'echo $0'
>>>> sh
>>>
>>> Hey Laurent,
>>>
>>> We have quite some time before the v5.6 merge window opens. So I would
>>> really like for this new feature to come with proper testing!
>>
>> Are there some already existing tests for binfmt_misc or namespace I can
>> update to test the new feature?
>
> I don't think so but there are tests for other namespace-aware
> filesystem. For example, I've added basic tests for binderfs in
> tools/testing/selftests/filesystems/binderfs/ and there are some devpts
> tests in there (Though the devpts tests don't actually make use of the
> kselftest framework so they aren't a great example. I'm not claiming
> binderfs is either tbh. :))
>
> You can just place the binfmt_misc tests in there. Helpers for setting
> up user namespace and mappings are in there as well. I think you can
> just place them in a separate file/header and include it for both
> binderfs and binfmt_misc.
> I'm happy to review this/answer questions.
>
OK, thank you, I will try to add some tests here.
Thanks,
Laurent
^ permalink raw reply
* Re: [PATCH v7 1/1] ns: add binfmt_misc to the user namespace
From: Michael Kerrisk (man-pages) @ 2019-12-16 15:34 UTC (permalink / raw)
To: Laurent Vivier
Cc: James Bottomley, Henning Schild, lkml, Dmitry Safonov,
linux-fsdevel@vger.kernel.org, Eric Biederman, Linux API,
Andrei Vagin, Cédric Le Goater, Greg Kurz, Jann Horn,
Containers, Alexander Viro, Jan Kiszka
In-Reply-To: <dbd19cb9-9172-d89d-f796-05a23213ca69@vivier.eu>
Hi Laurent,
On Mon, 16 Dec 2019 at 10:13, Laurent Vivier <laurent@vivier.eu> wrote:
>
> Le 14/12/2019 à 13:32, Michael Kerrisk (man-pages) a écrit :
> > Hello Laurent,
> >
> > On Sat, 14 Dec 2019 at 12:35, Laurent Vivier <laurent@vivier.eu> wrote:
> >>
> >> Le 13/12/2019 à 20:59, James Bottomley a écrit :
> >>> On Fri, 2019-12-13 at 18:51 +0100, Henning Schild wrote:
> >>>> Hi all,
> >>>>
> >>>> that is a very useful contribution, which will hopefully be
> >>>> considered.
> >>>
> >>> I'm technically the maintainer on the you touched it last you own it
> >>> basis, so if Christian's concerns get addressed I'll shepherd it
> >>> upstream.
> >>
> >> Thank you.
> >>
> >> I update this in the next days and re-send the patch.
> >
> > Would you also be so kind as to craft a patch for the
> > user_namespaces(7) manual page describing the changes (sent to me,
> > linux-man@vger.kernel.org, and the other parties already in CC)?
> >
> > If you do not have the time to familiarize yourself with groff/man
> > markup, a patch that uses plain text is fine; I can handle the
> > formatting.
>
> I will send a patch for the user_namespaces(7) manual.
Thanks! Could you send that in parallel with (each of) the patch
iterations please (rather than after the feature has been merged).
Thanks,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
^ permalink raw reply
* RE: [PATCH] openat2: switch to __attribute__((packed)) for open_how
From: David Laight @ 2019-12-16 16:55 UTC (permalink / raw)
To: 'Aleksa Sarai', Rasmus Villemoes
Cc: Alexander Viro, Jeff Layton, J. Bruce Fields, Shuah Khan,
dev@opencontainers.org, containers@lists.linux-foundation.org,
linux-api@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org
In-Reply-To: <20191215123443.jmfnrtgbscdwfohc@yavin.dot.cyphar.com>
From: Aleksa Sarai
> Sent: 15 December 2019 12:35
> On 2019-12-14, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> > On 13/12/2019 23.23, Aleksa Sarai wrote:
> > > The design of the original open_how struct layout was such that it
> > > ensured that there would be no un-labelled (and thus potentially
> > > non-zero) padding to avoid issues with struct expansion, as well as
> > > providing a uniform representation on all architectures (to avoid
> > > complications with OPEN_HOW_SIZE versioning).
> > >
> > > However, there were a few other desirable features which were not
> > > fulfilled by the previous struct layout:
> > >
> > > * Adding new features (other than new flags) should always result in
> > > the struct getting larger. However, by including a padding field, it
> > > was possible for new fields to be added without expanding the
> > > structure. This would somewhat complicate version-number based
> > > checking of feature support.
> > >
> > > * A non-zero bit in __padding yielded -EINVAL when it should arguably
> > > have been -E2BIG (because the padding bits are effectively
> > > yet-to-be-used fields). However, the semantics are not entirely clear
> > > because userspace may expect -E2BIG to only signify that the
> > > structure is too big. It's much simpler to just provide the guarantee
> > > that new fields will always result in a struct size increase, and
> > > -E2BIG indicates you're using a field that's too recent for an older
> > > kernel.
> >
> > And when the first extension adds another u64 field, that padding has to
> > be added back in and checked for being 0, at which point the padding is
> > again yet-to-be-used fields.
>
> Maybe I'm missing something, but what is the issue with
>
> struct open_how {
> u64 flags;
> u64 resolve;
> u16 mode;
> u64 next_extension;
> } __attribute__((packed));
Compile anything that accesses it for (say) sparc and look at the object code.
You really, really, REALLY, don't want to EVER use 'packed'.
Just use u64 for all the fields.
Use 'flags' bits to indicate whether the additional fields should be looked at.
Error if a 'flags' bit requires a value that isn't passed in the structure.
Then you can add an extra field and old source code recompiled with the
new headers will still work - because the 'junk' value isn't looked at.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH v8 1/1] ns: add binfmt_misc to the user namespace
From: Jann Horn @ 2019-12-16 19:08 UTC (permalink / raw)
To: Laurent Vivier
Cc: kernel list, Greg Kurz, Andrei Vagin, Linux API, Dmitry Safonov,
James Bottomley, Jan Kiszka, Christian Brauner, linux-fsdevel,
Linux Containers, Alexander Viro, Eric Biederman, Henning Schild,
Cédric Le Goater
In-Reply-To: <20191216091220.465626-2-laurent@vivier.eu>
On Mon, Dec 16, 2019 at 10:12 AM Laurent Vivier <laurent@vivier.eu> wrote:
> This patch allows to have a different binfmt_misc configuration
> for each new user namespace. By default, the binfmt_misc configuration
> is the one of the previous level, but if the binfmt_misc filesystem is
> mounted in the new namespace a new empty binfmt instance is created and
> used in this namespace.
>
> For instance, using "unshare" we can start a chroot of another
> architecture and configure the binfmt_misc interpreter without being root
> to run the binaries in this chroot.
How do you ensure that when userspace is no longer using the user
namespace and mount namespace, the entries and the binfmt_misc
superblock are deleted? As far as I can tell from looking at the code,
at the moment, if I create a user namespace+mount namespace, mount
binfmt_misc in there, register a file format and then let all
processes inside the namespaces exit, the binfmt_misc mount will be
kept alive by the simple_pin_fs() stuff, and the binfmt_misc entries
will also stay in memory.
[...]
> @@ -718,7 +736,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> if (!inode)
> goto out2;
>
> - err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count);
> + ns = binfmt_ns(file_dentry(file)->d_sb->s_user_ns);
> + err = simple_pin_fs(&bm_fs_type, &ns->bm_mnt,
> + &ns->entry_count);
When you call simple_pin_fs() here, the user namespace of `current`
and the user namespace of the superblock are not necessarily related.
So simple_pin_fs() may end up taking a reference on the mountpoint for
a user namespace that has nothing to do with the namespace for which
an entry is being created.
[...]
> static int bm_fill_super(struct super_block *sb, struct fs_context *fc)
> {
> int err;
> + struct user_namespace *ns = sb->s_user_ns;
[...]
> + /* create a new binfmt namespace
> + * if we are not in the first user namespace
> + * but the binfmt namespace is the first one
> + */
> + if (READ_ONCE(ns->binfmt_ns) == NULL) {
The READ_ONCE() here is unnecessary, right? AFAIK the VFS layer is
going to ensure that bm_fill_super() can't run concurrently for the
same namespace?
> + struct binfmt_namespace *new_ns;
> +
> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
> + GFP_KERNEL);
> + if (new_ns == NULL)
> + return -ENOMEM;
> + INIT_LIST_HEAD(&new_ns->entries);
> + new_ns->enabled = 1;
> + rwlock_init(&new_ns->entries_lock);
> + new_ns->bm_mnt = NULL;
> + new_ns->entry_count = 0;
> + /* ensure new_ns is completely initialized before sharing it */
> + smp_wmb();
> + WRITE_ONCE(ns->binfmt_ns, new_ns);
Nit: This would be a little bit semantically clearer if you used
smp_store_release() instead of smp_wmb()+WRITE_ONCE().
> + }
> +
> err = simple_fill_super(sb, BINFMTFS_MAGIC, bm_files);
[...]
> +static void bm_free(struct fs_context *fc)
> +{
> + if (fc->s_fs_info)
> + put_user_ns(fc->s_fs_info);
> +}
Silly question: Why the "if"? Can you ever reach this with fc->s_fs_info==NULL?
> +
> static int bm_get_tree(struct fs_context *fc)
> {
> - return get_tree_single(fc, bm_fill_super);
> + return get_tree_keyed(fc, bm_fill_super, get_user_ns(fc->user_ns));
get_user_ns() increments the refcount of the namespace, but in the
case where a binfmt_misc mount already exists, that refcount is never
dropped, right? That would be a security bug, since an attacker could
overflow the refcount of the user namespace and then trigger a UAF.
(And the refcount hardening won't catch it because user namespaces
still use raw atomics instead of refcount_t.)
[...]
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
Nit: Isn't this kind of check normally written as "#ifdef"?
^ permalink raw reply
* Re: [PATCH v18 11/13] open: introduce openat2(2) syscall
From: Florian Weimer @ 2019-12-16 19:20 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
Jonathan Corbet, linux-ia64, linux-doc, Alexander Shishkin,
Rasmus Villemoes, linux-kernel, linux-kselftest, sparclinux,
linux-api, Jiri Olsa
In-Reply-To: <20191206141338.23338-12-cyphar@cyphar.com>
* Aleksa Sarai:
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index 1d338357df8a..58c3a0e543c6 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -93,5 +93,40 @@
>
> #define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
>
> +/*
> + * Arguments for how openat2(2) should open the target path. If @resolve is
> + * zero, then openat2(2) operates very similarly to openat(2).
> + *
> + * However, unlike openat(2), unknown bits in @flags result in -EINVAL rather
> + * than being silently ignored. @mode must be zero unless one of {O_CREAT,
> + * O_TMPFILE} are set.
> + *
> + * @flags: O_* flags.
> + * @mode: O_CREAT/O_TMPFILE file mode.
> + * @resolve: RESOLVE_* flags.
> + */
> +struct open_how {
> + __aligned_u64 flags;
> + __u16 mode;
> + __u16 __padding[3]; /* must be zeroed */
> + __aligned_u64 resolve;
> +};
> +
> +#define OPEN_HOW_SIZE_VER0 24 /* sizeof first published struct */
> +#define OPEN_HOW_SIZE_LATEST OPEN_HOW_SIZE_VER0
> +
> +/* how->resolve flags for openat2(2). */
> +#define RESOLVE_NO_XDEV 0x01 /* Block mount-point crossings
> + (includes bind-mounts). */
> +#define RESOLVE_NO_MAGICLINKS 0x02 /* Block traversal through procfs-style
> + "magic-links". */
> +#define RESOLVE_NO_SYMLINKS 0x04 /* Block traversal through all symlinks
> + (implies OEXT_NO_MAGICLINKS) */
> +#define RESOLVE_BENEATH 0x08 /* Block "lexical" trickery like
> + "..", symlinks, and absolute
> + paths which escape the dirfd. */
> +#define RESOLVE_IN_ROOT 0x10 /* Make all jumps to "/" and ".."
> + be scoped inside the dirfd
> + (similar to chroot(2)). */
>
> #endif /* _UAPI_LINUX_FCNTL_H */
Would it be possible to move these to a new UAPI header?
In glibc, we currently do not #include <linux/fcntl.h>. We need some of
the AT_* constants in POSIX mode, and the header is not necessarily
namespace-clean. If there was a separate header for openat2 support, we
could use that easily, and we would only have to maintain the baseline
definitions (which never change).
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH v8 1/1] ns: add binfmt_misc to the user namespace
From: Laurent Vivier @ 2019-12-16 20:05 UTC (permalink / raw)
To: Jann Horn
Cc: kernel list, Greg Kurz, Andrei Vagin, Linux API, Dmitry Safonov,
James Bottomley, Jan Kiszka, Christian Brauner, linux-fsdevel,
Linux Containers, Alexander Viro, Eric Biederman, Henning Schild,
Cédric Le Goater
In-Reply-To: <CAG48ez2xNCRmuzpNqYW5R+XMKzW8YiemsPUPgk42KSkSZXmvLg@mail.gmail.com>
Le 16/12/2019 à 20:08, Jann Horn a écrit :
> On Mon, Dec 16, 2019 at 10:12 AM Laurent Vivier <laurent@vivier.eu> wrote:
>> This patch allows to have a different binfmt_misc configuration
>> for each new user namespace. By default, the binfmt_misc configuration
>> is the one of the previous level, but if the binfmt_misc filesystem is
>> mounted in the new namespace a new empty binfmt instance is created and
>> used in this namespace.
>>
>> For instance, using "unshare" we can start a chroot of another
>> architecture and configure the binfmt_misc interpreter without being root
>> to run the binaries in this chroot.
>
> How do you ensure that when userspace is no longer using the user
> namespace and mount namespace, the entries and the binfmt_misc
> superblock are deleted? As far as I can tell from looking at the code,
> at the moment, if I create a user namespace+mount namespace, mount
> binfmt_misc in there, register a file format and then let all
> processes inside the namespaces exit, the binfmt_misc mount will be
> kept alive by the simple_pin_fs() stuff, and the binfmt_misc entries
> will also stay in memory.
>
> [...]
Do you have any idea how I can fix this issue?
>> @@ -718,7 +736,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
>> if (!inode)
>> goto out2;
>>
>> - err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count);
>> + ns = binfmt_ns(file_dentry(file)->d_sb->s_user_ns);
>> + err = simple_pin_fs(&bm_fs_type, &ns->bm_mnt,
>> + &ns->entry_count);
>
> When you call simple_pin_fs() here, the user namespace of `current`
> and the user namespace of the superblock are not necessarily related.
> So simple_pin_fs() may end up taking a reference on the mountpoint for
> a user namespace that has nothing to do with the namespace for which
> an entry is being created.
Do you have any idea how I can fix this issue?
>
> [...]
>> static int bm_fill_super(struct super_block *sb, struct fs_context *fc)
>> {
>> int err;
>> + struct user_namespace *ns = sb->s_user_ns;
> [...]
>> + /* create a new binfmt namespace
>> + * if we are not in the first user namespace
>> + * but the binfmt namespace is the first one
>> + */
>> + if (READ_ONCE(ns->binfmt_ns) == NULL) {
>
> The READ_ONCE() here is unnecessary, right? AFAIK the VFS layer is
> going to ensure that bm_fill_super() can't run concurrently for the
> same namespace?
So I understand the "READ_ONCE()" is unnecessary and I will remove it.
>
>> + struct binfmt_namespace *new_ns;
>> +
>> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
>> + GFP_KERNEL);
>> + if (new_ns == NULL)
>> + return -ENOMEM;
>> + INIT_LIST_HEAD(&new_ns->entries);
>> + new_ns->enabled = 1;
>> + rwlock_init(&new_ns->entries_lock);
>> + new_ns->bm_mnt = NULL;
>> + new_ns->entry_count = 0;
>> + /* ensure new_ns is completely initialized before sharing it */
>> + smp_wmb();
>> + WRITE_ONCE(ns->binfmt_ns, new_ns);
>
> Nit: This would be a little bit semantically clearer if you used
> smp_store_release() instead of smp_wmb()+WRITE_ONCE().
I will.
>
>> + }
>> +
>> err = simple_fill_super(sb, BINFMTFS_MAGIC, bm_files);
> [...]
>> +static void bm_free(struct fs_context *fc)
>> +{
>> + if (fc->s_fs_info)
>> + put_user_ns(fc->s_fs_info);
>> +}
>
> Silly question: Why the "if"? Can you ever reach this with fc->s_fs_info==NULL?
So I understand the if is unnecessary and I will remove it.
>
>> +
>> static int bm_get_tree(struct fs_context *fc)
>> {
>> - return get_tree_single(fc, bm_fill_super);
>> + return get_tree_keyed(fc, bm_fill_super, get_user_ns(fc->user_ns));
>
> get_user_ns() increments the refcount of the namespace, but in the
> case where a binfmt_misc mount already exists, that refcount is never
> dropped, right? That would be a security bug, since an attacker could
> overflow the refcount of the user namespace and then trigger a UAF.
> (And the refcount hardening won't catch it because user namespaces
> still use raw atomics instead of refcount_t.)
Do you have any idea how I can fix this issue?
> [...]
>> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
>
> Nit: Isn't this kind of check normally written as "#ifdef"?
>
What is the difference?
Thanks,
Laurent
^ permalink raw reply
* Re: [PATCH v8 1/1] ns: add binfmt_misc to the user namespace
From: Jann Horn @ 2019-12-16 22:53 UTC (permalink / raw)
To: Laurent Vivier
Cc: kernel list, Greg Kurz, Andrei Vagin, Linux API, Dmitry Safonov,
James Bottomley, Jan Kiszka, Christian Brauner, linux-fsdevel,
Linux Containers, Alexander Viro, Eric Biederman, Henning Schild,
Cédric Le Goater
In-Reply-To: <15d270a6-2264-adc5-3f56-fdb8b67ad267@vivier.eu>
On Mon, Dec 16, 2019 at 9:05 PM Laurent Vivier <laurent@vivier.eu> wrote:
> Le 16/12/2019 à 20:08, Jann Horn a écrit :
> > On Mon, Dec 16, 2019 at 10:12 AM Laurent Vivier <laurent@vivier.eu> wrote:
> >> This patch allows to have a different binfmt_misc configuration
> >> for each new user namespace. By default, the binfmt_misc configuration
> >> is the one of the previous level, but if the binfmt_misc filesystem is
> >> mounted in the new namespace a new empty binfmt instance is created and
> >> used in this namespace.
> >>
> >> For instance, using "unshare" we can start a chroot of another
> >> architecture and configure the binfmt_misc interpreter without being root
> >> to run the binaries in this chroot.
> >
> > How do you ensure that when userspace is no longer using the user
> > namespace and mount namespace, the entries and the binfmt_misc
> > superblock are deleted? As far as I can tell from looking at the code,
> > at the moment, if I create a user namespace+mount namespace, mount
> > binfmt_misc in there, register a file format and then let all
> > processes inside the namespaces exit, the binfmt_misc mount will be
> > kept alive by the simple_pin_fs() stuff, and the binfmt_misc entries
> > will also stay in memory.
> >
> > [...]
>
> Do you have any idea how I can fix this issue?
I think the easiest way (keeping in mind that we want to avoid having
to fiddle around with reference loops, where e.g. interpreter
executable files opened by binfmt_misc have references back to the
user namespace through ->f_cred) would be to add a new patch in front
of this one that changes the semantics such that when binfmt_misc is
unmounted, all the existing format registrations are deleted. That's
probably also nicer from the perspective of inspectability. It could
in theory break stuff, but I think that's probably somewhat unlikely.
Still, it'd be an API change, and therefore you should CC linux-api@
on such a change.
> >> @@ -718,7 +736,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> >> if (!inode)
> >> goto out2;
> >>
> >> - err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count);
> >> + ns = binfmt_ns(file_dentry(file)->d_sb->s_user_ns);
> >> + err = simple_pin_fs(&bm_fs_type, &ns->bm_mnt,
> >> + &ns->entry_count);
> >
> > When you call simple_pin_fs() here, the user namespace of `current`
> > and the user namespace of the superblock are not necessarily related.
> > So simple_pin_fs() may end up taking a reference on the mountpoint for
> > a user namespace that has nothing to do with the namespace for which
> > an entry is being created.
>
> Do you have any idea how I can fix this issue?
If you fix the memory leak the way I suggested, this wouldn't be a
problem anymore either.
> >> +static void bm_free(struct fs_context *fc)
> >> +{
> >> + if (fc->s_fs_info)
> >> + put_user_ns(fc->s_fs_info);
> >> +}
> >
> > Silly question: Why the "if"? Can you ever reach this with fc->s_fs_info==NULL?
>
> So I understand the if is unnecessary and I will remove it.
Your code was actually exactly right, I didn't understand how
fc->s_fs_info works.
> >> static int bm_get_tree(struct fs_context *fc)
> >> {
> >> - return get_tree_single(fc, bm_fill_super);
> >> + return get_tree_keyed(fc, bm_fill_super, get_user_ns(fc->user_ns));
> >
> > get_user_ns() increments the refcount of the namespace, but in the
> > case where a binfmt_misc mount already exists, that refcount is never
> > dropped, right? That would be a security bug, since an attacker could
> > overflow the refcount of the user namespace and then trigger a UAF.
> > (And the refcount hardening won't catch it because user namespaces
> > still use raw atomics instead of refcount_t.)
>
> Do you have any idea how I can fix this issue?
Ah, this was actually fine. I missed that get_tree_keyed() stashes
that pointer in fc->s_fs_info.
> >> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> >
> > Nit: Isn't this kind of check normally written as "#ifdef"?
> >
>
> What is the difference?
As explained in Documentation/process/coding-style.rst and the
relevant header, IS_ENABLED() is for inline use in C expressions.
^ permalink raw reply
* [PATCH v3 0/4] Add pidfd getfd ioctl (Was Add ptrace get_fd request)
From: Sargun Dhillon @ 2019-12-17 0:58 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro,
gpascutto, ealvarez, fweimer, jld
This patchset introduces a mechanism to capture file descriptors from other
processes by pidfd and ioctl. Although this can be achieved using
SCM_RIGHTS, and parasitic code injection, this offers a more
straightforward mechanism.
It has a flags mechanism that's only usable to set CLOEXEC on the fd,
but I'm thinking that it could be extended to other aspects. For example,
for sockets, one could want to scrub the cgroup information.
Changes since v2:
* Move to ioctl on pidfd instead of ptrace function
* Add security check before moving file descriptor
Changes since the RFC v1:
* Introduce a new helper to fs/file.c to fetch a file descriptor from
any process. It largely uses the code suggested by Oleg, with a few
changes to fix locking
* It uses an extensible options struct to supply the FD, and option.
* I added a sample, using the code from the user-ptrace sample
Sargun Dhillon (4):
vfs, fdtable: Add get_task_file helper
pid: Add PIDFD_IOCTL_GETFD to fetch file descriptors from processes
samples: split generalized user-trap code into helper file
samples: Add example of using pidfd getfd in conjunction with user
trap
Documentation/ioctl/ioctl-number.rst | 1 +
fs/file.c | 22 +++-
include/linux/file.h | 2 +
include/linux/pid.h | 1 +
include/uapi/linux/pid.h | 26 ++++
kernel/fork.c | 72 ++++++++++
samples/seccomp/.gitignore | 1 +
samples/seccomp/Makefile | 15 ++-
samples/seccomp/user-trap-helper.c | 84 ++++++++++++
samples/seccomp/user-trap-helper.h | 13 ++
samples/seccomp/user-trap-pidfd.c | 190 +++++++++++++++++++++++++++
samples/seccomp/user-trap.c | 85 +-----------
12 files changed, 424 insertions(+), 88 deletions(-)
create mode 100644 include/uapi/linux/pid.h
create mode 100644 samples/seccomp/user-trap-helper.c
create mode 100644 samples/seccomp/user-trap-helper.h
create mode 100644 samples/seccomp/user-trap-pidfd.c
--
2.20.1
^ permalink raw reply
* [PATCH v3 2/4] pid: Add PIDFD_IOCTL_GETFD to fetch file descriptors from processes
From: Sargun Dhillon @ 2019-12-17 1:00 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro,
gpascutto, ealvarez, fweimer, jld
This adds an ioctl which allows file descriptors to be extracted
from processes based on their pidfd.
One reason to use this is to allow sandboxers to take actions on file
descriptors on the behalf of another process. For example, this can be
combined with seccomp-bpf's user notification to do on-demand fd
extraction and take privileged actions. For example, it can be used
to bind a socket to a privileged port. This is similar to ptrace, and
using ptrace parasitic code injection to extract a file descriptor from a
process, but without breaking debuggers, or paying the ptrace overhead
cost.
You must have the ability to ptrace the process in order to extract any
file descriptors from it. ptrace can already be used to extract file
descriptors based on parasitic code injections, so the permissions
model is aligned.
The ioctl takes a pointer to pidfd_getfd_args. pidfd_getfd_args contains
a size, which allows for gradual evolution of the API. There is an options
field, which can be used to state whether the fd should be opened with
CLOEXEC, or not. An additional options field may be added in the future
to include the ability to clear cgroup information about the file
descriptor at a later point. If the structure is from a newer kernel, and
includes members which make it larger than the structure that's known to
this kernel version, E2BIG will be returned.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
Documentation/ioctl/ioctl-number.rst | 1 +
include/linux/pid.h | 1 +
include/uapi/linux/pid.h | 26 ++++++++++
kernel/fork.c | 72 ++++++++++++++++++++++++++++
4 files changed, 100 insertions(+)
create mode 100644 include/uapi/linux/pid.h
diff --git a/Documentation/ioctl/ioctl-number.rst b/Documentation/ioctl/ioctl-number.rst
index bef79cd4c6b4..be2efb93acd1 100644
--- a/Documentation/ioctl/ioctl-number.rst
+++ b/Documentation/ioctl/ioctl-number.rst
@@ -272,6 +272,7 @@ Code Seq# Include File Comments
<mailto:tim@cyberelk.net>
'p' A1-A5 linux/pps.h LinuxPPS
<mailto:giometti@linux.it>
+'p' B0-CF uapi/linux/pid.h
'q' 00-1F linux/serio.h
'q' 80-FF linux/telephony.h Internet PhoneJACK, Internet LineJACK
linux/ixjuser.h <http://web.archive.org/web/%2A/http://www.quicknet.net>
diff --git a/include/linux/pid.h b/include/linux/pid.h
index 9645b1194c98..65f1a73040c9 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -5,6 +5,7 @@
#include <linux/rculist.h>
#include <linux/wait.h>
#include <linux/refcount.h>
+#include <uapi/linux/pid.h>
enum pid_type
{
diff --git a/include/uapi/linux/pid.h b/include/uapi/linux/pid.h
new file mode 100644
index 000000000000..4ec02ed8b39a
--- /dev/null
+++ b/include/uapi/linux/pid.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_LINUX_PID_H
+#define _UAPI_LINUX_PID_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+/* options to pass in to pidfd_getfd_args flags */
+#define PIDFD_GETFD_CLOEXEC (1 << 0) /* open the fd with cloexec */
+
+struct pidfd_getfd_args {
+ __u32 size; /* sizeof(pidfd_getfd_args) */
+ __u32 fd; /* the tracee's file descriptor to get */
+ __u32 flags;
+};
+
+#define PIDFD_IOC_MAGIC 'p'
+#define PIDFD_IO(nr) _IO(PIDFD_IOC_MAGIC, nr)
+#define PIDFD_IOR(nr, type) _IOR(PIDFD_IOC_MAGIC, nr, type)
+#define PIDFD_IOW(nr, type) _IOW(PIDFD_IOC_MAGIC, nr, type)
+#define PIDFD_IOWR(nr, type) _IOWR(PIDFD_IOC_MAGIC, nr, type)
+
+#define PIDFD_IOCTL_GETFD PIDFD_IOWR(0xb0, \
+ struct pidfd_getfd_args)
+
+#endif /* _UAPI_LINUX_PID_H */
diff --git a/kernel/fork.c b/kernel/fork.c
index 6cabc124378c..d9971e664e82 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1726,9 +1726,81 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
return poll_flags;
}
+static long pidfd_getfd(struct pid *pid, struct pidfd_getfd_args __user *buf)
+{
+ struct pidfd_getfd_args args;
+ unsigned int fd_flags = 0;
+ struct task_struct *task;
+ struct file *file;
+ u32 user_size;
+ int ret, fd;
+
+ ret = get_user(user_size, &buf->size);
+ if (ret)
+ return ret;
+
+ ret = copy_struct_from_user(&args, sizeof(args), buf, user_size);
+ if (ret)
+ return ret;
+ if ((args.flags & ~(PIDFD_GETFD_CLOEXEC)) != 0)
+ return -EINVAL;
+ if (args.flags & PIDFD_GETFD_CLOEXEC)
+ fd_flags |= O_CLOEXEC;
+
+ task = get_pid_task(pid, PIDTYPE_PID);
+ if (!task)
+ return -ESRCH;
+ ret = -EPERM;
+ if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
+ goto out;
+ ret = -EBADF;
+ file = fget_task(task, args.fd);
+ if (!file)
+ goto out;
+
+ fd = get_unused_fd_flags(fd_flags);
+ if (fd < 0) {
+ ret = fd;
+ goto out_put_file;
+ }
+ /*
+ * security_file_receive must come last since it may have side effects
+ * and cannot be reversed.
+ */
+ ret = security_file_receive(file);
+ if (ret)
+ goto out_put_fd;
+
+ fd_install(fd, file);
+ put_task_struct(task);
+ return fd;
+
+out_put_fd:
+ put_unused_fd(fd);
+out_put_file:
+ fput(file);
+out:
+ put_task_struct(task);
+ return ret;
+}
+
+static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ struct pid *pid = file->private_data;
+ void __user *buf = (void __user *)arg;
+
+ switch (cmd) {
+ case PIDFD_IOCTL_GETFD:
+ return pidfd_getfd(pid, buf);
+ default:
+ return -EINVAL;
+ }
+}
+
const struct file_operations pidfd_fops = {
.release = pidfd_release,
.poll = pidfd_poll,
+ .unlocked_ioctl = pidfd_ioctl,
#ifdef CONFIG_PROC_FS
.show_fdinfo = pidfd_show_fdinfo,
#endif
--
2.20.1
^ permalink raw reply related
* [PATCH v3 3/4] samples: split generalized user-trap code into helper file
From: Sargun Dhillon @ 2019-12-17 1:00 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro,
gpascutto, ealvarez, fweimer, jld
This moves the code for setting up a syscall interceptor with user
notification and sending the user notification file descriptor over a
socket using SCM_RIGHTS into a file that can be shared between multiple
samples.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
samples/seccomp/Makefile | 6 ++-
samples/seccomp/user-trap-helper.c | 84 +++++++++++++++++++++++++++++
samples/seccomp/user-trap-helper.h | 13 +++++
samples/seccomp/user-trap.c | 85 +-----------------------------
4 files changed, 103 insertions(+), 85 deletions(-)
create mode 100644 samples/seccomp/user-trap-helper.c
create mode 100644 samples/seccomp/user-trap-helper.h
diff --git a/samples/seccomp/Makefile b/samples/seccomp/Makefile
index 009775b52538..82b7347318d1 100644
--- a/samples/seccomp/Makefile
+++ b/samples/seccomp/Makefile
@@ -16,9 +16,13 @@ HOSTCFLAGS_bpf-direct.o += -I$(objtree)/usr/include
HOSTCFLAGS_bpf-direct.o += -idirafter $(objtree)/include
bpf-direct-objs := bpf-direct.o
+
+HOSTCFLAGS_user-trap-helper.o += -I$(objtree)/usr/include
+HOSTCFLAGS_user-trap-helper.o += -idirafter $(objtree)/include
+
HOSTCFLAGS_user-trap.o += -I$(objtree)/usr/include
HOSTCFLAGS_user-trap.o += -idirafter $(objtree)/include
-user-trap-objs := user-trap.o
+user-trap-objs := user-trap.o user-trap-helper.o
# Try to match the kernel target.
ifndef CONFIG_64BIT
diff --git a/samples/seccomp/user-trap-helper.c b/samples/seccomp/user-trap-helper.c
new file mode 100644
index 000000000000..f91ae9d947c5
--- /dev/null
+++ b/samples/seccomp/user-trap-helper.c
@@ -0,0 +1,84 @@
+#include <linux/seccomp.h>
+#include <linux/filter.h>
+#include <unistd.h>
+#include <errno.h>
+#include <stdio.h>
+#include <stddef.h>
+#include <sys/types.h>
+#include <sys/syscall.h>
+#include <sys/socket.h>
+#include "user-trap-helper.h"
+
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
+
+int user_trap_syscall(int nr, unsigned int flags)
+{
+ struct sock_filter filter[] = {
+ BPF_STMT(BPF_LD+BPF_W+BPF_ABS,
+ offsetof(struct seccomp_data, nr)),
+ BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, nr, 0, 1),
+ BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_USER_NOTIF),
+ BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ALLOW),
+ };
+
+ struct sock_fprog prog = {
+ .len = (unsigned short)ARRAY_SIZE(filter),
+ .filter = filter,
+ };
+
+ return seccomp(SECCOMP_SET_MODE_FILTER, flags, &prog);
+}
+
+int send_fd(int sock, int fd)
+{
+ struct msghdr msg = {};
+ struct cmsghdr *cmsg;
+ char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
+ struct iovec io = {
+ .iov_base = &c,
+ .iov_len = 1,
+ };
+
+ msg.msg_iov = &io;
+ msg.msg_iovlen = 1;
+ msg.msg_control = buf;
+ msg.msg_controllen = sizeof(buf);
+ cmsg = CMSG_FIRSTHDR(&msg);
+ cmsg->cmsg_level = SOL_SOCKET;
+ cmsg->cmsg_type = SCM_RIGHTS;
+ cmsg->cmsg_len = CMSG_LEN(sizeof(int));
+ *((int *)CMSG_DATA(cmsg)) = fd;
+ msg.msg_controllen = cmsg->cmsg_len;
+
+ if (sendmsg(sock, &msg, 0) < 0) {
+ perror("sendmsg");
+ return -1;
+ }
+
+ return 0;
+}
+
+int recv_fd(int sock)
+{
+ struct msghdr msg = {};
+ struct cmsghdr *cmsg;
+ char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
+ struct iovec io = {
+ .iov_base = &c,
+ .iov_len = 1,
+ };
+
+ msg.msg_iov = &io;
+ msg.msg_iovlen = 1;
+ msg.msg_control = buf;
+ msg.msg_controllen = sizeof(buf);
+
+ if (recvmsg(sock, &msg, 0) < 0) {
+ perror("recvmsg");
+ return -1;
+ }
+
+ cmsg = CMSG_FIRSTHDR(&msg);
+
+ return *((int *)CMSG_DATA(cmsg));
+}
diff --git a/samples/seccomp/user-trap-helper.h b/samples/seccomp/user-trap-helper.h
new file mode 100644
index 000000000000..a5ebda25fdfe
--- /dev/null
+++ b/samples/seccomp/user-trap-helper.h
@@ -0,0 +1,13 @@
+#include <unistd.h>
+#include <sys/syscall.h>
+#include <errno.h>
+
+static inline int seccomp(unsigned int op, unsigned int flags, void *args)
+{
+ errno = 0;
+ return syscall(__NR_seccomp, op, flags, args);
+}
+
+int user_trap_syscall(int nr, unsigned int flags);
+int send_fd(int sock, int fd);
+int recv_fd(int sock);
diff --git a/samples/seccomp/user-trap.c b/samples/seccomp/user-trap.c
index 6d0125ca8af7..1b6526587456 100644
--- a/samples/seccomp/user-trap.c
+++ b/samples/seccomp/user-trap.c
@@ -5,101 +5,18 @@
#include <errno.h>
#include <fcntl.h>
#include <string.h>
-#include <stddef.h>
#include <sys/sysmacros.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <sys/socket.h>
#include <sys/stat.h>
#include <sys/mman.h>
-#include <sys/syscall.h>
#include <sys/user.h>
#include <sys/ioctl.h>
-#include <sys/ptrace.h>
#include <sys/mount.h>
#include <linux/limits.h>
-#include <linux/filter.h>
#include <linux/seccomp.h>
-
-#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
-
-static int seccomp(unsigned int op, unsigned int flags, void *args)
-{
- errno = 0;
- return syscall(__NR_seccomp, op, flags, args);
-}
-
-static int send_fd(int sock, int fd)
-{
- struct msghdr msg = {};
- struct cmsghdr *cmsg;
- char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
- struct iovec io = {
- .iov_base = &c,
- .iov_len = 1,
- };
-
- msg.msg_iov = &io;
- msg.msg_iovlen = 1;
- msg.msg_control = buf;
- msg.msg_controllen = sizeof(buf);
- cmsg = CMSG_FIRSTHDR(&msg);
- cmsg->cmsg_level = SOL_SOCKET;
- cmsg->cmsg_type = SCM_RIGHTS;
- cmsg->cmsg_len = CMSG_LEN(sizeof(int));
- *((int *)CMSG_DATA(cmsg)) = fd;
- msg.msg_controllen = cmsg->cmsg_len;
-
- if (sendmsg(sock, &msg, 0) < 0) {
- perror("sendmsg");
- return -1;
- }
-
- return 0;
-}
-
-static int recv_fd(int sock)
-{
- struct msghdr msg = {};
- struct cmsghdr *cmsg;
- char buf[CMSG_SPACE(sizeof(int))] = {0}, c = 'c';
- struct iovec io = {
- .iov_base = &c,
- .iov_len = 1,
- };
-
- msg.msg_iov = &io;
- msg.msg_iovlen = 1;
- msg.msg_control = buf;
- msg.msg_controllen = sizeof(buf);
-
- if (recvmsg(sock, &msg, 0) < 0) {
- perror("recvmsg");
- return -1;
- }
-
- cmsg = CMSG_FIRSTHDR(&msg);
-
- return *((int *)CMSG_DATA(cmsg));
-}
-
-static int user_trap_syscall(int nr, unsigned int flags)
-{
- struct sock_filter filter[] = {
- BPF_STMT(BPF_LD+BPF_W+BPF_ABS,
- offsetof(struct seccomp_data, nr)),
- BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, nr, 0, 1),
- BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_USER_NOTIF),
- BPF_STMT(BPF_RET+BPF_K, SECCOMP_RET_ALLOW),
- };
-
- struct sock_fprog prog = {
- .len = (unsigned short)ARRAY_SIZE(filter),
- .filter = filter,
- };
-
- return seccomp(SECCOMP_SET_MODE_FILTER, flags, &prog);
-}
+#include "user-trap-helper.h"
static int handle_req(struct seccomp_notif *req,
struct seccomp_notif_resp *resp, int listener)
--
2.20.1
^ permalink raw reply related
* [PATCH v3 4/4] samples: Add example of using pidfd getfd in conjunction with user trap
From: Sargun Dhillon @ 2019-12-17 1:00 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro,
gpascutto, ealvarez, fweimer, jld
This sample adds the usage of SECCOMP_RET_USER_NOTIF together with pidfd
GETFD ioctl. It shows trapping a syscall, and handling it by extracting
the FD into the parent process without stopping the child process.
Although, in this example, there's no explicit policy separation in
the two processes, it can be generalized into the example of a transparent
proxy.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
samples/seccomp/.gitignore | 1 +
samples/seccomp/Makefile | 9 +-
samples/seccomp/user-trap-pidfd.c | 190 ++++++++++++++++++++++++++++++
3 files changed, 199 insertions(+), 1 deletion(-)
create mode 100644 samples/seccomp/user-trap-pidfd.c
diff --git a/samples/seccomp/.gitignore b/samples/seccomp/.gitignore
index d1e2e817d556..f37e3692a1dd 100644
--- a/samples/seccomp/.gitignore
+++ b/samples/seccomp/.gitignore
@@ -2,3 +2,4 @@ bpf-direct
bpf-fancy
dropper
user-trap
+user-trap-pidfd
diff --git a/samples/seccomp/Makefile b/samples/seccomp/Makefile
index 82b7347318d1..c3880869cadc 100644
--- a/samples/seccomp/Makefile
+++ b/samples/seccomp/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
ifndef CROSS_COMPILE
-hostprogs-y := bpf-fancy dropper bpf-direct user-trap
+hostprogs-y := bpf-fancy dropper bpf-direct user-trap user-trap-pidfd
HOSTCFLAGS_bpf-fancy.o += -I$(objtree)/usr/include
HOSTCFLAGS_bpf-fancy.o += -idirafter $(objtree)/include
@@ -24,6 +24,11 @@ HOSTCFLAGS_user-trap.o += -I$(objtree)/usr/include
HOSTCFLAGS_user-trap.o += -idirafter $(objtree)/include
user-trap-objs := user-trap.o user-trap-helper.o
+HOSTCFLAGS_user-trap-pidfd.o += -I$(objtree)/usr/include
+HOSTCFLAGS_user-trap-pidfd.o += -idirafter $(objtree)/include
+user-trap-pidfd-objs := user-trap-pidfd.o user-trap-helper.o
+
+
# Try to match the kernel target.
ifndef CONFIG_64BIT
@@ -39,10 +44,12 @@ HOSTCFLAGS_dropper.o += $(MFLAG)
HOSTCFLAGS_bpf-helper.o += $(MFLAG)
HOSTCFLAGS_bpf-fancy.o += $(MFLAG)
HOSTCFLAGS_user-trap.o += $(MFLAG)
+HOSTCFLAGS_user-trap-pidfd.o += $(MFLAG)
HOSTLDLIBS_bpf-direct += $(MFLAG)
HOSTLDLIBS_bpf-fancy += $(MFLAG)
HOSTLDLIBS_dropper += $(MFLAG)
HOSTLDLIBS_user-trap += $(MFLAG)
+HOSTLDLIBS_user-trap-pidfd += $(MFLAG)
endif
always := $(hostprogs-y)
endif
diff --git a/samples/seccomp/user-trap-pidfd.c b/samples/seccomp/user-trap-pidfd.c
new file mode 100644
index 000000000000..960e58982063
--- /dev/null
+++ b/samples/seccomp/user-trap-pidfd.c
@@ -0,0 +1,190 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/seccomp.h>
+#include <linux/prctl.h>
+#include <linux/pid.h>
+#include <sys/socket.h>
+#include <sys/prctl.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <sys/ioctl.h>
+#include <assert.h>
+#include <errno.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+#include <netinet/in.h>
+#include "user-trap-helper.h"
+
+#define CHILD_PORT_TRY_BIND 80
+#define CHILD_PORT_ACTUAL_BIND 4998
+
+static int tracee(void)
+{
+ struct sockaddr_in addr = {
+ .sin_family = AF_INET,
+ .sin_port = htons(CHILD_PORT_TRY_BIND),
+ .sin_addr = {
+ .s_addr = htonl(INADDR_ANY)
+ }
+ };
+ socklen_t addrlen = sizeof(addr);
+ int sock, ret = 1;
+
+ sock = socket(AF_INET, SOCK_STREAM, 0);
+ if (sock == -1) {
+ perror("socket");
+ goto out;
+ }
+
+
+ if (bind(sock, (struct sockaddr *) &addr, sizeof(addr))) {
+ perror("bind");
+ goto out;
+ }
+
+ printf("Child successfully performed bind operation\n");
+ if (getsockname(sock, (struct sockaddr *) &addr, &addrlen)) {
+ perror("getsockname");
+ goto out;
+ }
+
+
+ printf("Socket bound to port %d\n", ntohs(addr.sin_port));
+ assert(ntohs(addr.sin_port) == CHILD_PORT_ACTUAL_BIND);
+
+ ret = 0;
+out:
+ return ret;
+}
+
+static int handle_req(int listener, int pidfd)
+{
+ struct sockaddr_in addr = {
+ .sin_family = AF_INET,
+ .sin_port = htons(CHILD_PORT_ACTUAL_BIND),
+ .sin_addr = {
+ .s_addr = htonl(INADDR_LOOPBACK)
+ }
+ };
+ struct pidfd_getfd_args getfd_args = {
+ .flags = PIDFD_GETFD_CLOEXEC
+ };
+ struct seccomp_notif_sizes sizes;
+ struct seccomp_notif_resp *resp;
+ struct seccomp_notif *req;
+ int fd, ret = 1;
+
+ getfd_args.size = sizeof(getfd_args);
+ if (seccomp(SECCOMP_GET_NOTIF_SIZES, 0, &sizes) < 0) {
+ perror("seccomp(GET_NOTIF_SIZES)");
+ goto out;
+ }
+ req = malloc(sizes.seccomp_notif);
+ if (!req)
+ goto out;
+ memset(req, 0, sizeof(*req));
+
+ resp = malloc(sizes.seccomp_notif_resp);
+ if (!resp)
+ goto out_free_req;
+ memset(resp, 0, sizeof(*resp));
+
+ if (ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, req)) {
+ perror("ioctl recv");
+ goto out;
+ }
+ printf("Child tried to call bind with fd: %lld\n", req->data.args[0]);
+ getfd_args.fd = req->data.args[0];
+ fd = ioctl(pidfd, PIDFD_IOCTL_GETFD, &getfd_args);
+ if (fd == -1) {
+ perror("ioctl pidfd");
+ goto out_free_resp;
+ }
+ if (bind(fd, (struct sockaddr *) &addr, sizeof(addr))) {
+ perror("bind");
+ goto out_free_resp;
+ }
+
+ resp->id = req->id;
+ resp->error = 0;
+ resp->val = 0;
+ if (ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, resp) < 0) {
+ perror("ioctl send");
+ goto out_free_resp;
+ }
+
+ ret = 0;
+out_free_resp:
+ free(resp);
+out_free_req:
+ free(req);
+out:
+ return ret;
+}
+
+static int pidfd_open(pid_t pid, unsigned int flags)
+{
+ errno = 0;
+ return syscall(__NR_pidfd_open, pid, flags);
+}
+
+int main(void)
+{
+ int pidfd, listener, sk_pair[2], ret = 1;
+ pid_t pid;
+
+ if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0) {
+ perror("socketpair");
+ goto out;
+ }
+
+ pid = fork();
+ if (pid < 0) {
+ perror("fork");
+ goto close_pair;
+ }
+
+ if (pid == 0) {
+ if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) {
+ perror("prctl(NO_NEW_PRIVS)");
+ exit(1);
+ }
+ listener = user_trap_syscall(__NR_bind,
+ SECCOMP_FILTER_FLAG_NEW_LISTENER);
+ if (listener < 0) {
+ perror("seccomp");
+ exit(1);
+ }
+ if (send_fd(sk_pair[1], listener) < 0)
+ exit(1);
+ close(listener);
+ exit(tracee());
+ }
+
+ pidfd = pidfd_open(pid, 0);
+ if (pidfd < 0) {
+ perror("pidfd_open");
+ goto kill_child;
+ }
+
+ listener = recv_fd(sk_pair[0]);
+ if (listener < 0)
+ goto kill_child;
+
+ if (handle_req(listener, pidfd))
+ goto kill_child;
+
+ /* Wait for child to finish */
+ waitpid(pid, NULL, 0);
+
+ ret = 0;
+ goto close_pair;
+
+kill_child:
+ kill(pid, SIGKILL);
+close_pair:
+ close(sk_pair[0]);
+ close(sk_pair[1]);
+out:
+ return ret;
+}
--
2.20.1
^ permalink raw reply related
* [PATCH v3 1/4] vfs, fdtable: Add get_task_file helper
From: Sargun Dhillon @ 2019-12-17 1:01 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: tycho, jannh, cyphar, christian.brauner, oleg, luto, viro,
gpascutto, ealvarez, fweimer, jld
This introduces a function which can be used to fetch a file, given an
arbitrary task. As long as the user holds a reference (refcnt) to the
task_struct it is safe to call, and will either return NULL on failure,
or a pointer to the file, with a refcnt.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
fs/file.c | 22 ++++++++++++++++++++--
include/linux/file.h | 2 ++
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/fs/file.c b/fs/file.c
index 3da91a112bab..63272d15be61 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -706,9 +706,9 @@ void do_close_on_exec(struct files_struct *files)
spin_unlock(&files->file_lock);
}
-static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs)
+static struct file *__fget_files(struct files_struct *files, unsigned int fd,
+ fmode_t mask, unsigned int refs)
{
- struct files_struct *files = current->files;
struct file *file;
rcu_read_lock();
@@ -729,6 +729,11 @@ static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs)
return file;
}
+static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs)
+{
+ return __fget_files(current->files, fd, mask, refs);
+}
+
struct file *fget_many(unsigned int fd, unsigned int refs)
{
return __fget(fd, FMODE_PATH, refs);
@@ -746,6 +751,19 @@ struct file *fget_raw(unsigned int fd)
}
EXPORT_SYMBOL(fget_raw);
+struct file *fget_task(struct task_struct *task, unsigned int fd)
+{
+ struct file *file = NULL;
+
+ task_lock(task);
+ if (task->files)
+ file = __fget_files(task->files, fd, 0, 1);
+
+ task_unlock(task);
+
+ return file;
+}
+
/*
* Lightweight file lookup - no refcnt increment if fd table isn't shared.
*
diff --git a/include/linux/file.h b/include/linux/file.h
index 3fcddff56bc4..c6c7b24ea9f7 100644
--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -16,6 +16,7 @@ extern void fput(struct file *);
extern void fput_many(struct file *, unsigned int);
struct file_operations;
+struct task_struct;
struct vfsmount;
struct dentry;
struct inode;
@@ -47,6 +48,7 @@ static inline void fdput(struct fd fd)
extern struct file *fget(unsigned int fd);
extern struct file *fget_many(unsigned int fd, unsigned int refs);
extern struct file *fget_raw(unsigned int fd);
+extern struct file *fget_task(struct task_struct *task, unsigned int fd);
extern unsigned long __fdget(unsigned int fd);
extern unsigned long __fdget_raw(unsigned int fd);
extern unsigned long __fdget_pos(unsigned int fd);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v3 2/4] pid: Add PIDFD_IOCTL_GETFD to fetch file descriptors from processes
From: Jann Horn @ 2019-12-17 1:13 UTC (permalink / raw)
To: Sargun Dhillon
Cc: kernel list, Linux Containers, Linux API, linux-fsdevel,
Tycho Andersen, Aleksa Sarai, Christian Brauner, Oleg Nesterov,
Andy Lutomirski, Al Viro, gpascutto, ealvarez, Florian Weimer,
jld
In-Reply-To: <20191217010001.GA14461@ircssh-2.c.rugged-nimbus-611.internal>
On Tue, Dec 17, 2019 at 2:00 AM Sargun Dhillon <sargun@sargun.me> wrote:
> This adds an ioctl which allows file descriptors to be extracted
> from processes based on their pidfd.
[...]
> You must have the ability to ptrace the process in order to extract any
> file descriptors from it. ptrace can already be used to extract file
> descriptors based on parasitic code injections, so the permissions
> model is aligned.
[...]
> + task = get_pid_task(pid, PIDTYPE_PID);
> + if (!task)
> + return -ESRCH;
> + ret = -EPERM;
Please add something like
if (mutex_lock_killable(&task->signal->cred_guard_mutex))
goto out;
here, and drop the mutex after fget_task().
> + if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
> + goto out;
> + ret = -EBADF;
> + file = fget_task(task, args.fd);
> + if (!file)
> + goto out;
> +
> + fd = get_unused_fd_flags(fd_flags);
> + if (fd < 0) {
> + ret = fd;
> + goto out_put_file;
> + }
> + /*
> + * security_file_receive must come last since it may have side effects
> + * and cannot be reversed.
> + */
> + ret = security_file_receive(file);
> + if (ret)
> + goto out_put_fd;
> +
> + fd_install(fd, file);
> + put_task_struct(task);
> + return fd;
> +
> +out_put_fd:
> + put_unused_fd(fd);
> +out_put_file:
> + fput(file);
> +out:
> + put_task_struct(task);
> + return ret;
> +}
^ permalink raw reply
* Re: [PATCH v3 2/4] pid: Add PIDFD_IOCTL_GETFD to fetch file descriptors from processes
From: Christian Brauner @ 2019-12-17 1:50 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, containers, linux-api, linux-fsdevel, tycho, jannh,
cyphar, oleg, luto, viro, gpascutto, ealvarez, fweimer, jld, arnd
In-Reply-To: <20191217010001.GA14461@ircssh-2.c.rugged-nimbus-611.internal>
[Cc Arnd since he fiddled with ioctl()s quite a bit recently.]
On Tue, Dec 17, 2019 at 01:00:04AM +0000, Sargun Dhillon wrote:
> This adds an ioctl which allows file descriptors to be extracted
> from processes based on their pidfd.
>
> One reason to use this is to allow sandboxers to take actions on file
> descriptors on the behalf of another process. For example, this can be
> combined with seccomp-bpf's user notification to do on-demand fd
> extraction and take privileged actions. For example, it can be used
> to bind a socket to a privileged port. This is similar to ptrace, and
> using ptrace parasitic code injection to extract a file descriptor from a
> process, but without breaking debuggers, or paying the ptrace overhead
> cost.
>
> You must have the ability to ptrace the process in order to extract any
> file descriptors from it. ptrace can already be used to extract file
> descriptors based on parasitic code injections, so the permissions
> model is aligned.
>
> The ioctl takes a pointer to pidfd_getfd_args. pidfd_getfd_args contains
> a size, which allows for gradual evolution of the API. There is an options
> field, which can be used to state whether the fd should be opened with
> CLOEXEC, or not. An additional options field may be added in the future
> to include the ability to clear cgroup information about the file
> descriptor at a later point. If the structure is from a newer kernel, and
> includes members which make it larger than the structure that's known to
> this kernel version, E2BIG will be returned.
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> ---
> Documentation/ioctl/ioctl-number.rst | 1 +
> include/linux/pid.h | 1 +
> include/uapi/linux/pid.h | 26 ++++++++++
> kernel/fork.c | 72 ++++++++++++++++++++++++++++
> 4 files changed, 100 insertions(+)
> create mode 100644 include/uapi/linux/pid.h
>
> diff --git a/Documentation/ioctl/ioctl-number.rst b/Documentation/ioctl/ioctl-number.rst
> index bef79cd4c6b4..be2efb93acd1 100644
> --- a/Documentation/ioctl/ioctl-number.rst
> +++ b/Documentation/ioctl/ioctl-number.rst
> @@ -272,6 +272,7 @@ Code Seq# Include File Comments
> <mailto:tim@cyberelk.net>
> 'p' A1-A5 linux/pps.h LinuxPPS
> <mailto:giometti@linux.it>
> +'p' B0-CF uapi/linux/pid.h
> 'q' 00-1F linux/serio.h
> 'q' 80-FF linux/telephony.h Internet PhoneJACK, Internet LineJACK
> linux/ixjuser.h <http://web.archive.org/web/%2A/http://www.quicknet.net>
> diff --git a/include/linux/pid.h b/include/linux/pid.h
> index 9645b1194c98..65f1a73040c9 100644
> --- a/include/linux/pid.h
> +++ b/include/linux/pid.h
> @@ -5,6 +5,7 @@
> #include <linux/rculist.h>
> #include <linux/wait.h>
> #include <linux/refcount.h>
> +#include <uapi/linux/pid.h>
That should be pidfd.h and the resulting new file be placed under the
pidfd entry in maintainers:
+F: include/uapi/linux/pidfd.h
>
> enum pid_type
> {
> diff --git a/include/uapi/linux/pid.h b/include/uapi/linux/pid.h
> new file mode 100644
> index 000000000000..4ec02ed8b39a
> --- /dev/null
> +++ b/include/uapi/linux/pid.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +#ifndef _UAPI_LINUX_PID_H
> +#define _UAPI_LINUX_PID_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +/* options to pass in to pidfd_getfd_args flags */
> +#define PIDFD_GETFD_CLOEXEC (1 << 0) /* open the fd with cloexec */
Please, make them cloexec by default unless there's a very good reason
not to.
> +
> +struct pidfd_getfd_args {
> + __u32 size; /* sizeof(pidfd_getfd_args) */
> + __u32 fd; /* the tracee's file descriptor to get */
> + __u32 flags;
> +};
I think you want to either want to pad this
+struct pidfd_getfd_args {
+ __u32 size; /* sizeof(pidfd_getfd_args) */
+ __u32 fd; /* the tracee's file descriptor to get */
+ __u32 flags;
__u32 reserved;
+};
or use __aligned_u64 everywhere which I'd personally prefer instead of
this manual padding everywhere.
> +
> +#define PIDFD_IOC_MAGIC 'p'
> +#define PIDFD_IO(nr) _IO(PIDFD_IOC_MAGIC, nr)
> +#define PIDFD_IOR(nr, type) _IOR(PIDFD_IOC_MAGIC, nr, type)
> +#define PIDFD_IOW(nr, type) _IOW(PIDFD_IOC_MAGIC, nr, type)
> +#define PIDFD_IOWR(nr, type) _IOWR(PIDFD_IOC_MAGIC, nr, type)
> +
> +#define PIDFD_IOCTL_GETFD PIDFD_IOWR(0xb0, \
> + struct pidfd_getfd_args)
> +
> +#endif /* _UAPI_LINUX_PID_H */
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 6cabc124378c..d9971e664e82 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1726,9 +1726,81 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
> return poll_flags;
> }
>
> +static long pidfd_getfd(struct pid *pid, struct pidfd_getfd_args __user *buf)
> +{
> + struct pidfd_getfd_args args;
> + unsigned int fd_flags = 0;
> + struct task_struct *task;
> + struct file *file;
> + u32 user_size;
> + int ret, fd;
> +
> + ret = get_user(user_size, &buf->size);
> + if (ret)
> + return ret;
> +
> + ret = copy_struct_from_user(&args, sizeof(args), buf, user_size);
> + if (ret)
> + return ret;
> + if ((args.flags & ~(PIDFD_GETFD_CLOEXEC)) != 0)
> + return -EINVAL;
Nit: It's more common - especially in this file - to do
if (args.flags & ~PIDFD_GETFD_CLOEXEC)
return -EINVAL;
> + if (args.flags & PIDFD_GETFD_CLOEXEC)
> + fd_flags |= O_CLOEXEC;
> +
> + task = get_pid_task(pid, PIDTYPE_PID);
> + if (!task)
> + return -ESRCH;
\n
> + ret = -EPERM;
> + if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
> + goto out;
\n
Please don't pre-set errors unless they are used by multiple exit paths.
if (!ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS)) {
ret = -EPERM;
goto out;
}
> + ret = -EBADF;
> + file = fget_task(task, args.fd);
> + if (!file)
> + goto out;
Same.
> +
> + fd = get_unused_fd_flags(fd_flags);
> + if (fd < 0) {
> + ret = fd;
> + goto out_put_file;
> + }
\n
> + /*
> + * security_file_receive must come last since it may have side effects
> + * and cannot be reversed.
> + */
> + ret = security_file_receive(file);
> + if (ret)
> + goto out_put_fd;
> +
> + fd_install(fd, file);
> + put_task_struct(task);
> + return fd;
> +
> +out_put_fd:
> + put_unused_fd(fd);
> +out_put_file:
> + fput(file);
> +out:
> + put_task_struct(task);
> + return ret;
> +}
> +
> +static long pidfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> + struct pid *pid = file->private_data;
> + void __user *buf = (void __user *)arg;
> +
> + switch (cmd) {
> + case PIDFD_IOCTL_GETFD:
> + return pidfd_getfd(pid, buf);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> const struct file_operations pidfd_fops = {
> .release = pidfd_release,
> .poll = pidfd_poll,
> + .unlocked_ioctl = pidfd_ioctl,
> #ifdef CONFIG_PROC_FS
> .show_fdinfo = pidfd_show_fdinfo,
> #endif
> --
> 2.20.1
>
^ 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