Skip to content

Fix panic when starting container with invalid device cgroup rule#41919

Merged
thaJeztah merged 1 commit into
moby:masterfrom
thaJeztah:fix_cgroup_rule_panic
Feb 4, 2021
Merged

Fix panic when starting container with invalid device cgroup rule#41919
thaJeztah merged 1 commit into
moby:masterfrom
thaJeztah:fix_cgroup_rule_panic

Conversation

@thaJeztah

Copy link
Copy Markdown
Member

fixes/addresses docker/compose#8052

This fixes a panic when an invalid "device cgroup rule" is passed, resulting in an "index out of range".

This bug was introduced in the original implementation in 1756af6 (#22563), but was not reproducible when using the CLI, because the same commit also added client-side validation on the flag before making an API request. The following example, uses an invalid rule (c *:* rwm - two spaces before the permissions);

$ docker run --rm --network=host --device-cgroup-rule='c *:*  rwm' busybox
invalid argument "c *:*  rwm" for "--device-cgroup-rule" flag: invalid device cgroup format 'c *:*  rwm'

Doing the same, but using the API results in a daemon panic when starting the container;

  1. Create a container with an invalid device cgroup rule:
$ curl -v \
  --unix-socket /var/run/docker.sock \
  "http://localhost/v1.41/containers/create?name=foobar" \
  -H "Content-Type: application/json" \
  -d '{"Image":"busybox:latest", "HostConfig":{"DeviceCgroupRules": ["c *:*  rwm"]}}'
  1. Start the container:
$ curl -v \
  --unix-socket /var/run/docker.sock \
  -X POST \
  "http://localhost/v1.41/containers/foobar/start"
  1. Observe the daemon logs:
2021-01-22 12:53:03.313806 I | http: panic serving @: runtime error: index out of range [0] with length 0
goroutine 571 [running]:
net/http.(*conn).serve.func1(0xc000cb2d20)
	/usr/local/go/src/net/http/server.go:1795 +0x13b
panic(0x2f32380, 0xc000aebfc0)
	/usr/local/go/src/runtime/panic.go:679 +0x1b6
github.com/docker/docker/oci.AppendDevicePermissionsFromCgroupRules(0xc000175c00, 0x8, 0x8, 0xc0000bd380, 0x1, 0x4, 0x0, 0x0, 0xc0000e69c0, 0x0, ...)
	/go/src/github.com/docker/docker/oci/oci.go:34 +0x64f

This patch:

  • fixes the panic, allowing the daemon to return an error on container start
  • adds a unit-test to validate various permutations
  • adds a "todo" to verify the regular expression (and handling) of the "a" (all) value

We should also consider performing this validation when creating the container,
so that an error is produced early.

- Description for the changelog

Fix daemon panic when starting container with invalid device cgroup rule

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah thaJeztah added this to the 20.10.3 milestone Jan 22, 2021
@thaJeztah

thaJeztah commented Jan 22, 2021

Copy link
Copy Markdown
Member Author

Some follow-ups I want to do;

  • Look into the accepted formats ("TODO" I added)
  • We may be able to continue accepting a with other options, but should ignore the devices and permissions (or produce a sensible error)
  • Looks like we currently don't invalidate negative values (e.g. c -5:-5 rwm) (-1 could be acceptable as it's equivalent to *) actually, this is already handled by the regex, but looks like we may not validate against the actual max values (https://github.com/torvalds/linux/blob/v5.10/include/linux/kdev_t.h)
  • Look into moving this validation to "container create" (we may have to keep it in the oci/ as well, to account for existing container specs?
    • Once that's done, ideally, remove the validation from the client side

@thaJeztah

Copy link
Copy Markdown
Member Author

@cpuguy83 @AkihiroSuda @mlaventure ptal

/cc @dmm (thanks for reporting the issue), and @aiordache (thanks for the "ping" on slack)

Comment thread oci/oci_test.go Outdated

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is to check the validation, but validation will have to be updated to reflect the correct range; https://github.com/torvalds/linux/blob/v5.10/include/linux/kdev_t.h

@thaJeztah

Copy link
Copy Markdown
Member Author

Opened #41920 to track follow-up enhancements

@thaJeztah

Copy link
Copy Markdown
Member Author

argh; forgot to push a change I made in the test. hold on

This fixes a panic when an invalid "device cgroup rule" is passed, resulting
in an "index out of range".

This bug was introduced in the original implementation in 1756af6,
but was not reproducible when using the CLI, because the same commit also added
client-side validation on the flag before making an API request. The following
example, uses an invalid rule (`c *:*  rwm` - two spaces before the permissions);

```console
$ docker run --rm --network=host --device-cgroup-rule='c *:*  rwm' busybox
invalid argument "c *:*  rwm" for "--device-cgroup-rule" flag: invalid device cgroup format 'c *:*  rwm'
```

Doing the same, but using the API results in a daemon panic when starting the container;

Create a container with an invalid device cgroup rule:

```console
curl -v \
  --unix-socket /var/run/docker.sock \
  "http://localhost/v1.41/containers/create?name=foobar" \
  -H "Content-Type: application/json" \
  -d '{"Image":"busybox:latest", "HostConfig":{"DeviceCgroupRules": ["c *:*  rwm"]}}'
```

Start the container:

```console
curl -v \
  --unix-socket /var/run/docker.sock \
  -X POST \
  "http://localhost/v1.41/containers/foobar/start"
```

Observe the daemon logs:

```
2021-01-22 12:53:03.313806 I | http: panic serving @: runtime error: index out of range [0] with length 0
goroutine 571 [running]:
net/http.(*conn).serve.func1(0xc000cb2d20)
	/usr/local/go/src/net/http/server.go:1795 +0x13b
panic(0x2f32380, 0xc000aebfc0)
	/usr/local/go/src/runtime/panic.go:679 +0x1b6
github.com/docker/docker/oci.AppendDevicePermissionsFromCgroupRules(0xc000175c00, 0x8, 0x8, 0xc0000bd380, 0x1, 0x4, 0x0, 0x0, 0xc0000e69c0, 0x0, ...)
	/go/src/github.com/docker/docker/oci/oci.go:34 +0x64f
```

This patch:

- fixes the panic, allowing the daemon to return an error on container start
- adds a unit-test to validate various permutations
- adds a "todo" to verify the regular expression (and handling) of the "a" (all) value

We should also consider performing this validation when _creating_ the container,
so that an error is produced early.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
thaJeztah force-pushed the fix_cgroup_rule_panic branch from f6fe69d to 5cc1753 Compare January 22, 2021 15:03
@thaJeztah

Copy link
Copy Markdown
Member Author

s390 failure is unrelated; see #41933

@cpuguy83 cpuguy83 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah modified the milestones: 20.10.3, 20.10.4 Feb 2, 2021
@thaJeztah thaJeztah modified the milestones: 20.10.4, 21.xx Feb 2, 2021
@thaJeztah

Copy link
Copy Markdown
Member Author

s390x failures are unrelated; bringing this one in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants