Bug 90385 - nspawn: block devices passed to --bind/bind-ro are not accessible inside the container
Summary: nspawn: block devices passed to --bind/bind-ro are not accessible inside the ...
Status: RESOLVED FIXED
Alias: None
Product: systemd
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: Other Linux (All)
: medium normal
Assignee: systemd-bugs
QA Contact: systemd-bugs
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-10 00:59 UTC by Stefan Junker
Modified: 2015-05-14 20:56 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
patch - nspawn - DeviceAllow for block devices that are passed --bind/--bind-ro (4.57 KB, text/plain)
2015-05-10 00:59 UTC, Stefan Junker
Details
patch adapted according to Lennart's implementation suggestions (1.97 KB, patch)
2015-05-13 19:23 UTC, Stefan Junker
Details | Splinter Review
patch - nspawn - DeviceAllow for block and char devices that are passed --bind/--bind-ro (2.57 KB, patch)
2015-05-14 20:46 UTC, Stefan Junker
Details | Splinter Review

Description Stefan Junker 2015-05-10 00:59:52 UTC
Created attachment 115666 [details]
patch - nspawn - DeviceAllow for block devices that are passed --bind/--bind-ro

When systemd-nspawn is called with the --bind or --bind-ro arguments and passed a block device, e.g. /dev/mmcblk0p1, the device is not accessible inside the container due to cgroup restrictions.

Testing or more generally running software that is only supposed to access specific block devices would be a convenient use-case for containers, with virtual machines being the conservative method for this. The fact that --bind/--bind-ro supports renaming paths for mounting them inside the container could be useful for running predefined routines that use a fixed device name, which can be swapped very easily by simply changing an argument for the specific container instance.

Blocking access to all system block devices is a good safety measure. Providing users with the possibility to override this for specific devices is a good feature.

I've attached a patch which adds detection of block devices to the mount_bind(). For every blockdevice source file the DeviceAllow property will be set for the scope of the machine that is being started.
Comment 1 Lennart Poettering 2015-05-13 13:29:11 UTC
Hmm, so in general I am not really too keen on pretending we could support device access from inside nspawn correctly, given that that's hardly just about raw device nodes, it's also a matter of enumeration, udev/sysfs metadata and probing, and all that won't work without proper device virtualization in the kernel.

That said, I figure supporting whitelisting things for device nodes listed in --bind= is probably an OK thing to do. Hence I generally agree with the proposed patch. 

A few notes though:

I'd like to avoid registering the container first, and then adjusting the whitelist in a second method call. Please, let's add them to the whitelist right at the time when we register the machine, in register_machine(). TO implement this the nspawn parent should probably simply iterate through all bind mounts in register_machine(), stat() them, and adding all char and block devices it finds to the list of whitelisted devices.
Comment 2 Stefan Junker 2015-05-13 19:23:05 UTC
Created attachment 115749 [details] [review]
patch adapted according to Lennart's implementation suggestions

Thank you Lennart for the review and the suggestions on the implementation.
I have changed the patch accordingly and created a new attachment.
Comment 3 Lennart Poettering 2015-05-14 15:55:15 UTC
Comment on attachment 115749 [details] [review]
patch adapted according to Lennart's implementation suggestions

A couple of fixes:

pleas structly use 8ch space indenting, you appear to indent by 4ch sometimes.

More importantly though, the code for nspawn changed in current git quite a bind, arg_bind and arg_bind_ro do not exist like this anymore. Please rebase on current git.

Also, I think this should work the same way for character and block device nodes. i.e. just checking S_ISBLK() appears to be restrictive.

We currently have two functions is_symlink() and is_dir() in util.[ch]. I think it would be best to introduce is_device_node() in this style, and move the stat() check into it, and then look for S_ISBLK() || S_ISCHR()...
Comment 4 Stefan Junker 2015-05-14 20:46:44 UTC
Created attachment 115783 [details] [review]
patch - nspawn - DeviceAllow for block and char devices that are passed --bind/--bind-ro

I've updated the patch with the new implementation.
Thanks for the feedback Lennart, please let me know if this one is fine.
Comment 5 Lennart Poettering 2015-05-14 20:56:03 UTC
Applied! Thanks!


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.