Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Don't deny all devices when update cgroup resource #2205

Closed
wants to merge 1 commit into from
Closed

Don't deny all devices when update cgroup resource #2205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 68 additions & 7 deletions libcontainer/cgroups/fs/devices.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
package fs

import (
"strings"

"github.com/opencontainers/runc/libcontainer/cgroups"
"github.com/opencontainers/runc/libcontainer/cgroups/fscommon"
"github.com/opencontainers/runc/libcontainer/configs"
Expand All @@ -12,6 +14,12 @@ import (
type DevicesGroup struct {
}

type Empty struct{}

var (
defaultDevice = &configs.Device{Type: 'a', Major: -1, Minor: -1, Permissions: "rwm"}
)

func (s *DevicesGroup) Name() string {
return "devices"
}
Expand All @@ -32,29 +40,63 @@ func (s *DevicesGroup) Set(path string, cgroup *configs.Cgroup) error {
}

devices := cgroup.Resources.Devices
oldAllowedDevices, err := readDevicesExceptDefault(path)
if err != nil {
return err
}

if len(devices) > 0 {
for _, dev := range devices {
file := "devices.deny"
if dev.Allow {
file = "devices.allow"
}
if err := fscommon.WriteFile(path, file, dev.CgroupString()); err != nil {
return err

// For the second time set, we don't deny all devices
if dev.Type == defaultDevice.Type && len(oldAllowedDevices) != 0 {
file = ""
}
Copy link
Member

@cyphar cyphar Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I really don't like this, and after looking at it for a bit I think I'm convinced it isn't right. This code will ignore any update that contains a -- but there are valid use-cases for a other than "deny everything" (you can allow both character and block devices for a particular major number, for instance). But even if this check was dev == defaultDevice it'd still be wrong since you can also allow all devices (though you shouldn't) -- and that would be ignored.

I think the issue is that the caller shouldn't be specifying a default deny rule in the list which we then need to filter out -- that should be done as part of this module. So we figure out what rules we need to specify.

EDIT: My comment about a was wrong, I forgot that a always means everything, and that you can't specify both block and character devices.


if len(file) > 0 {
if err := fscommon.WriteFile(path, file, dev.CgroupString()); err != nil {
return err
}
}
}
return nil
}
if cgroup.Resources.AllowAllDevices != nil {
if *cgroup.Resources.AllowAllDevices == false {
Copy link
Member

@cyphar cyphar Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This isn't directly related to this patch (and is something we should fix separately), but I've hit this wall while trying to review it.

I've spent a while now trying to understand what the hell this whole AllowAllDevices, AllowedDevices and DeniedDevices stuff is supposed to do and how it interacts with Devices. It's been around forever in runc, isn't exposed in the runtime-spec, and isn't even generated by specconv. It appears to be only used for unit testing itself, which seems like a senseless thing to test. Honestly, this patch and many others would be much nicer if we just nuked this from orbit and just had Devices. When the hell would you ever not want AllowAllDevices == false!?

In the context of this patch, I'm not even sure how to review whether the semantics of the below code are correct -- if I'm correct in my understanding that nothing uses the code, how do we know if the changes make sense...

@crosbymichael Am I missing something? Will something really bad happen if we get rid of it? I remember the initProcessTime nightmare when we changed the type of a field in the libcontainer state JSON (which resulted in the whole "migration" fun) -- would something similar happen if we just purged this concept from the codebase?

@AkihiroSuda WDYT? (Since you were the last person to touch it, and I think @crosbymichael and I last looked at this code in 2016.)

if err := fscommon.WriteFile(path, "devices.deny", "a"); err != nil {
return err
// For the first time set, we deny all devices to initialize the cgroup
if len(oldAllowedDevices) == 0 {
if err := fscommon.WriteFile(path, "devices.deny", "a"); err != nil {
return err
}
}

for _, dev := range cgroup.Resources.AllowedDevices {
if err := fscommon.WriteFile(path, "devices.allow", dev.CgroupString()); err != nil {
return err
newAllowedDevices := make(map[string]Empty)
for _, dev := range cgroup.AllowedDevices {
newAllowedDevices[dev.CgroupString()] = Empty{}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this should loop over cgroup.Devices given my previous point about how AllowedDevices is actually not used by anything. But as I said, most of this logic needs to be torched -- I just wrote a very long block of text about how this patch doesn't handle denying no longer allowed devices, only to realise that this code block actually is used, and in theory does handle that problem...


// Deny no longer allowed devices
for cgroupString := range oldAllowedDevices {
if _, found := newAllowedDevices[cgroupString]; !found {
if err := fscommon.WriteFile(path, "devices.deny", cgroupString); err != nil {
return err
}
}
}

// Allow new devices
for cgroupString := range newAllowedDevices {
if _, found := oldAllowedDevices[cgroupString]; !found {
if err := fscommon.WriteFile(path, "devices.allow", cgroupString); err != nil {
return err
}
}
}

return nil
}

Expand All @@ -79,3 +121,22 @@ func (s *DevicesGroup) Remove(d *cgroupData) error {
func (s *DevicesGroup) GetStats(path string, stats *cgroups.Stats) error {
return nil
}

func readDevicesExceptDefault(path string) (allowed map[string]Empty, err error) {
cgroupData, err := fscommon.ReadFile(path, "devices.list")
if err != nil {
return nil, err
}

allowedDevices := make(map[string]Empty)
defaultDeviceString := defaultDevice.CgroupString()
for _, data := range strings.Split(cgroupData, "\n") {
// skip allow all devices
if len(data) == 0 || data == defaultDeviceString {
continue
}
allowedDevices[data] = Empty{}
}

return allowedDevices, nil
}
6 changes: 6 additions & 0 deletions libcontainer/cgroups/fs/devices_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ func TestDevicesSetAllow(t *testing.T) {
helper := NewCgroupTestUtil("devices", t)
defer helper.cleanup()

helper.writeFileContents(map[string]string{
"devices.list": "a *:* rwm",
})
helper.writeFileContents(map[string]string{
"devices.deny": "a",
})
Expand Down Expand Up @@ -76,6 +79,9 @@ func TestDevicesSetDeny(t *testing.T) {
helper := NewCgroupTestUtil("devices", t)
defer helper.cleanup()

helper.writeFileContents(map[string]string{
"devices.list": "a *:* rwm",
})
helper.writeFileContents(map[string]string{
"devices.allow": "a",
})
Expand Down