fix(vm): identify the jailed VMM by its root inode, not readlink
The jailer pivot_roots, so readlink(/proc/<pid>/root) is "/" and the previous scope check never matched a real jailed Firecracker: Reap skipped it and its cgroup stayed busy (seen in TestVMStubExecutor on a real host, invisible to the symlink-based unit test). Compare the process's root (stat follows the magic link into the VM's root) with <jail base>/<exec name>/<id>/root by device and inode. Tests: same inode through another path matches; our base with another id, a foreign base, no chroot and an unreadable root do not.
- commit
- 25328fc05fa73edbb3261a84faad8beb4955c8a3
- author
- brice · · 2026-10-12 22:06 UTC
- parent
- 982af7c
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
2 files · +51 -8
M internal/sandbox/vm/proc.go +29 -4
| @@ -101,9 +101,9 @@ func (sc scope) under(p string) bool { | |||
| 101 | 101 | // /proc directory is dir, or "". Besides IDFromCmdline: | |
| 102 | 102 | // - a jailer (--exec-file) must have been given our --chroot-base-dir; | |
| 103 | 103 | // - a Firecracker that carries only the jailer's --start-time-us (no | |
| 104 | - | // --api-sock) is the jailed VMM: its /proc/<pid>/root must be below our | |
| 105 | - | // jail base. Anything else with that flag and an id of our shape belongs to | |
| 106 | - | // someone else's jailer and is never touched. | |
| 104 | + | // --api-sock) is the jailed VMM: its root must be the chroot of that id | |
| 105 | + | // below our jail base (rootIsOurs). Anything else with that flag and an id | |
| 106 | + | // of our shape belongs to someone else's jailer and is never touched. | |
| 107 | 107 | // | |
| 108 | 108 | // A Firecracker started with --api-sock (no jailer) is ours by its id alone. | |
| 109 | 109 | func (sc scope) match(dir string, args []string) string { | |
| @@ -119,13 +119,38 @@ func (sc scope) match(dir string, args []string) string { | |||
| 119 | 119 | return a.id | |
| 120 | 120 | } | |
| 121 | 121 | case a.markers[MarkerStartTime]: | |
| 122 | - | if root, err := os.Readlink(filepath.Join(dir, "root")); err == nil && sc.under(root) { | |
| 122 | + | if sc.rootIsOurs(dir, a.id) { | |
| 123 | 123 | return a.id | |
| 124 | 124 | } | |
| 125 | 125 | } | |
| 126 | 126 | return "" | |
| 127 | 127 | } | |
| 128 | 128 | ||
| 129 | + | // rootIsOurs reports whether the process's root directory is the chroot the | |
| 130 | + | // jailer made for id below our jail base. The jailer pivot_roots into a new | |
| 131 | + | // mount namespace, so readlink(/proc/<pid>/root) says "/" and cannot be used: | |
| 132 | + | // stat follows the magic link into the VM's root, which is the very directory | |
| 133 | + | // <base>/<exec name>/<id>/root of the host (same device and inode). A root | |
| 134 | + | // whose path is lexically below our base is accepted too. | |
| 135 | + | func (sc scope) rootIsOurs(dir, id string) bool { | |
| 136 | + | if root, err := os.Readlink(filepath.Join(dir, "root")); err == nil && sc.under(root) { | |
| 137 | + | return true | |
| 138 | + | } | |
| 139 | + | rootFI, err := os.Stat(filepath.Join(dir, "root")) | |
| 140 | + | if err != nil { | |
| 141 | + | return false | |
| 142 | + | } | |
| 143 | + | for _, b := range sc.jailBases { | |
| 144 | + | execDirs, _ := os.ReadDir(b) | |
| 145 | + | for _, e := range execDirs { | |
| 146 | + | if fi, err := os.Stat(filepath.Join(b, e.Name(), id, "root")); err == nil && os.SameFile(fi, rootFI) { | |
| 147 | + | return true | |
| 148 | + | } | |
| 149 | + | } | |
| 150 | + | } | |
| 151 | + | return false | |
| 152 | + | } | |
| 153 | + | ||
| 129 | 154 | // vmmProcs scans a /proc tree for processes started by this runner for a VM: | |
| 130 | 155 | // the Firecracker process and the jailer. A process is ours when its command | |
| 131 | 156 | // line has "--id <id>" with an id of the form sandbox.NewID produces and one of | |
| @@ -101,9 +101,9 @@ func (sc scope) under(p string) bool { | |||||
| 101 | // /proc directory is dir, or "". Besides IDFromCmdline: | 101 | // /proc directory is dir, or "". Besides IDFromCmdline: | ||
| 102 | // - a jailer (--exec-file) must have been given our --chroot-base-dir; | 102 | // - a jailer (--exec-file) must have been given our --chroot-base-dir; | ||
| 103 | // - a Firecracker that carries only the jailer's --start-time-us (no | 103 | // - a Firecracker that carries only the jailer's --start-time-us (no | ||
| 104 | - | // --api-sock) is the jailed VMM: its /proc/<pid>/root must be below our | 104 | + | // --api-sock) is the jailed VMM: its root must be the chroot of that id |
| 105 | - | // jail base. Anything else with that flag and an id of our shape belongs to | 105 | + | // below our jail base (rootIsOurs). Anything else with that flag and an id |
| 106 | - | // someone else's jailer and is never touched. | 106 | + | // of our shape belongs to someone else's jailer and is never touched. |
| 107 | // | 107 | // | ||
| 108 | // A Firecracker started with --api-sock (no jailer) is ours by its id alone. | 108 | // A Firecracker started with --api-sock (no jailer) is ours by its id alone. | ||
| 109 | func (sc scope) match(dir string, args []string) string { | 109 | func (sc scope) match(dir string, args []string) string { | ||
| @@ -119,13 +119,38 @@ func (sc scope) match(dir string, args []string) string { | |||||
| 119 | return a.id | 119 | return a.id | ||
| 120 | } | 120 | } | ||
| 121 | case a.markers[MarkerStartTime]: | 121 | case a.markers[MarkerStartTime]: | ||
| 122 | - | if root, err := os.Readlink(filepath.Join(dir, "root")); err == nil && sc.under(root) { | 122 | + | if sc.rootIsOurs(dir, a.id) { |
| 123 | return a.id | 123 | return a.id | ||
| 124 | } | 124 | } | ||
| 125 | } | 125 | } | ||
| 126 | return "" | 126 | return "" | ||
| 127 | } | 127 | } | ||
| 128 | 128 | ||||
| 129 | + | // rootIsOurs reports whether the process's root directory is the chroot the | |||
| 130 | + | // jailer made for id below our jail base. The jailer pivot_roots into a new | |||
| 131 | + | // mount namespace, so readlink(/proc/<pid>/root) says "/" and cannot be used: | |||
| 132 | + | // stat follows the magic link into the VM's root, which is the very directory | |||
| 133 | + | // <base>/<exec name>/<id>/root of the host (same device and inode). A root | |||
| 134 | + | // whose path is lexically below our base is accepted too. | |||
| 135 | + | func (sc scope) rootIsOurs(dir, id string) bool { | |||
| 136 | + | if root, err := os.Readlink(filepath.Join(dir, "root")); err == nil && sc.under(root) { | |||
| 137 | + | return true | |||
| 138 | + | } | |||
| 139 | + | rootFI, err := os.Stat(filepath.Join(dir, "root")) | |||
| 140 | + | if err != nil { | |||
| 141 | + | return false | |||
| 142 | + | } | |||
| 143 | + | for _, b := range sc.jailBases { | |||
| 144 | + | execDirs, _ := os.ReadDir(b) | |||
| 145 | + | for _, e := range execDirs { | |||
| 146 | + | if fi, err := os.Stat(filepath.Join(b, e.Name(), id, "root")); err == nil && os.SameFile(fi, rootFI) { | |||
| 147 | + | return true | |||
| 148 | + | } | |||
| 149 | + | } | |||
| 150 | + | } | |||
| 151 | + | return false | |||
| 152 | + | } | |||
| 153 | + | ||||
| 129 | // vmmProcs scans a /proc tree for processes started by this runner for a VM: | 154 | // vmmProcs scans a /proc tree for processes started by this runner for a VM: | ||
| 130 | // the Firecracker process and the jailer. A process is ours when its command | 155 | // the Firecracker process and the jailer. A process is ours when its command | ||
| 131 | // line has "--id <id>" with an id of the form sandbox.NewID produces and one of | 156 | // line has "--id <id>" with an id of the form sandbox.NewID produces and one of | ||
M internal/sandbox/vm/vm_test.go +22 -4
| @@ -1184,17 +1184,35 @@ func TestScanIsScopedToOurJailBase(t *testing.T) { | |||
| 1184 | 1184 | } | |
| 1185 | 1185 | } | |
| 1186 | 1186 | ours := filepath.Join(jailBase(state), "firecracker", "er00112233", "root") | |
| 1187 | + | if err := os.MkdirAll(ours, 0o755); err != nil { | |
| 1188 | + | t.Fatal(err) | |
| 1189 | + | } | |
| 1190 | + | // What a pivot_root'ed jailed VMM looks like: its root is our chroot directory | |
| 1191 | + | // but readlink does not say so (it says "/"); a symlinked alias reproduces | |
| 1192 | + | // that: another path, same inode. | |
| 1193 | + | alias := filepath.Join(t.TempDir(), "alias") | |
| 1194 | + | if err := os.Symlink(ours, alias); err != nil { | |
| 1195 | + | t.Fatal(err) | |
| 1196 | + | } | |
| 1197 | + | // Our jail base has a chroot for another id: a foreign VMM must not match it. | |
| 1198 | + | if err := os.MkdirAll(filepath.Join(jailBase(state), "firecracker", "er44556677", "root"), 0o755); err != nil { | |
| 1199 | + | t.Fatal(err) | |
| 1200 | + | } | |
| 1201 | + | foreignRoot := filepath.Join(foreign, "jail", "firecracker", "er44556677", "root") | |
| 1202 | + | _ = os.MkdirAll(foreignRoot, 0o755) | |
| 1187 | 1203 | mk("100", ours, "/firecracker", "--id", "er00112233", "--start-time-us", "1", "--start-time-cpu-us", "2") | |
| 1188 | - | mk("101", filepath.Join(foreign, "jail", "firecracker", "er44556677", "root"), "/firecracker", "--id", "er44556677", "--start-time-us", "1") | |
| 1189 | - | mk("102", "/", "/firecracker", "--id", "er8899aabb", "--start-time-us", "1") // not chrooted at all | |
| 1190 | - | mk("103", "", "/firecracker", "--id", "er0000cccc", "--start-time-us", "1") // root unreadable | |
| 1204 | + | mk("101", foreignRoot, "/firecracker", "--id", "er44556677", "--start-time-us", "1") | |
| 1205 | + | mk("109", alias, "/firecracker", "--id", "er00112233", "--start-time-us", "1") // same inode, other path | |
| 1206 | + | mk("110", alias, "/firecracker", "--id", "er77777777", "--start-time-us", "1") // our dir, but not that id's chroot | |
| 1207 | + | mk("102", "/", "/firecracker", "--id", "er8899aabb", "--start-time-us", "1") // not chrooted at all | |
| 1208 | + | mk("103", "", "/firecracker", "--id", "er0000cccc", "--start-time-us", "1") // root unreadable | |
| 1191 | 1209 | mk("104", "/", "jailer", "--id", "er11111111", "--exec-file", "/fc", "--chroot-base-dir", jailBase(state)) | |
| 1192 | 1210 | mk("105", "/", "jailer", "--id", "er22222222", "--exec-file", "/fc", "--chroot-base-dir", filepath.Join(foreign, "jail")) | |
| 1193 | 1211 | mk("106", "/", "jailer", "--id", "er33333333", "--exec-file", "/fc") | |
| 1194 | 1212 | mk("107", ours+"/../../../../x", "/firecracker", "--id", "er55555555", "--start-time-us", "1") // lexical escape | |
| 1195 | 1213 | mk("108", "/", "firecracker", "--id", "er66666666", "--api-sock", "/x") // unjailed: by id | |
| 1196 | 1214 | got := vmmProcs(proc, sc) | |
| 1197 | - | want := map[int]string{100: "er00112233", 104: "er11111111", 108: "er66666666"} | |
| 1215 | + | want := map[int]string{100: "er00112233", 104: "er11111111", 108: "er66666666", 109: "er00112233"} | |
| 1198 | 1216 | if len(got) != len(want) { | |
| 1199 | 1217 | t.Fatalf("got %v want %v", got, want) | |
| 1200 | 1218 | } | |
| @@ -1184,17 +1184,35 @@ func TestScanIsScopedToOurJailBase(t *testing.T) { | |||||
| 1184 | } | 1184 | } | ||
| 1185 | } | 1185 | } | ||
| 1186 | ours := filepath.Join(jailBase(state), "firecracker", "er00112233", "root") | 1186 | ours := filepath.Join(jailBase(state), "firecracker", "er00112233", "root") | ||
| 1187 | + | if err := os.MkdirAll(ours, 0o755); err != nil { | |||
| 1188 | + | t.Fatal(err) | |||
| 1189 | + | } | |||
| 1190 | + | // What a pivot_root'ed jailed VMM looks like: its root is our chroot directory | |||
| 1191 | + | // but readlink does not say so (it says "/"); a symlinked alias reproduces | |||
| 1192 | + | // that: another path, same inode. | |||
| 1193 | + | alias := filepath.Join(t.TempDir(), "alias") | |||
| 1194 | + | if err := os.Symlink(ours, alias); err != nil { | |||
| 1195 | + | t.Fatal(err) | |||
| 1196 | + | } | |||
| 1197 | + | // Our jail base has a chroot for another id: a foreign VMM must not match it. | |||
| 1198 | + | if err := os.MkdirAll(filepath.Join(jailBase(state), "firecracker", "er44556677", "root"), 0o755); err != nil { | |||
| 1199 | + | t.Fatal(err) | |||
| 1200 | + | } | |||
| 1201 | + | foreignRoot := filepath.Join(foreign, "jail", "firecracker", "er44556677", "root") | |||
| 1202 | + | _ = os.MkdirAll(foreignRoot, 0o755) | |||
| 1187 | mk("100", ours, "/firecracker", "--id", "er00112233", "--start-time-us", "1", "--start-time-cpu-us", "2") | 1203 | mk("100", ours, "/firecracker", "--id", "er00112233", "--start-time-us", "1", "--start-time-cpu-us", "2") | ||
| 1188 | - | mk("101", filepath.Join(foreign, "jail", "firecracker", "er44556677", "root"), "/firecracker", "--id", "er44556677", "--start-time-us", "1") | 1204 | + | mk("101", foreignRoot, "/firecracker", "--id", "er44556677", "--start-time-us", "1") |
| 1189 | - | mk("102", "/", "/firecracker", "--id", "er8899aabb", "--start-time-us", "1") // not chrooted at all | 1205 | + | mk("109", alias, "/firecracker", "--id", "er00112233", "--start-time-us", "1") // same inode, other path |
| 1190 | - | mk("103", "", "/firecracker", "--id", "er0000cccc", "--start-time-us", "1") // root unreadable | 1206 | + | mk("110", alias, "/firecracker", "--id", "er77777777", "--start-time-us", "1") // our dir, but not that id's chroot |
| 1207 | + | mk("102", "/", "/firecracker", "--id", "er8899aabb", "--start-time-us", "1") // not chrooted at all | |||
| 1208 | + | mk("103", "", "/firecracker", "--id", "er0000cccc", "--start-time-us", "1") // root unreadable | |||
| 1191 | mk("104", "/", "jailer", "--id", "er11111111", "--exec-file", "/fc", "--chroot-base-dir", jailBase(state)) | 1209 | mk("104", "/", "jailer", "--id", "er11111111", "--exec-file", "/fc", "--chroot-base-dir", jailBase(state)) | ||
| 1192 | mk("105", "/", "jailer", "--id", "er22222222", "--exec-file", "/fc", "--chroot-base-dir", filepath.Join(foreign, "jail")) | 1210 | mk("105", "/", "jailer", "--id", "er22222222", "--exec-file", "/fc", "--chroot-base-dir", filepath.Join(foreign, "jail")) | ||
| 1193 | mk("106", "/", "jailer", "--id", "er33333333", "--exec-file", "/fc") | 1211 | mk("106", "/", "jailer", "--id", "er33333333", "--exec-file", "/fc") | ||
| 1194 | mk("107", ours+"/../../../../x", "/firecracker", "--id", "er55555555", "--start-time-us", "1") // lexical escape | 1212 | mk("107", ours+"/../../../../x", "/firecracker", "--id", "er55555555", "--start-time-us", "1") // lexical escape | ||
| 1195 | mk("108", "/", "firecracker", "--id", "er66666666", "--api-sock", "/x") // unjailed: by id | 1213 | mk("108", "/", "firecracker", "--id", "er66666666", "--api-sock", "/x") // unjailed: by id | ||
| 1196 | got := vmmProcs(proc, sc) | 1214 | got := vmmProcs(proc, sc) | ||
| 1197 | - | want := map[int]string{100: "er00112233", 104: "er11111111", 108: "er66666666"} | 1215 | + | want := map[int]string{100: "er00112233", 104: "er11111111", 108: "er66666666", 109: "er00112233"} |
| 1198 | if len(got) != len(want) { | 1216 | if len(got) != len(want) { | ||
| 1199 | t.Fatalf("got %v want %v", got, want) | 1217 | t.Fatalf("got %v want %v", got, want) | ||
| 1200 | } | 1218 | } | ||