From f7aff0aaa4320505933df838cf5b551b69d5e513 Mon Sep 17 00:00:00 2001 From: Kevin Krakauer Date: Wed, 10 Apr 2019 12:35:43 -0700 Subject: Allow threads with CAP_SYS_RESOURCE to raise hard rlimits. PiperOrigin-RevId: 242919489 Change-Id: Ie3267b3bcd8a54b54bc16a6556369a19e843376f --- pkg/sentry/kernel/fd_map_test.go | 8 ++++---- pkg/sentry/limits/limits.go | 8 ++++++-- pkg/sentry/limits/limits_test.go | 30 ++++++++++++++++++------------ pkg/sentry/limits/linux.go | 2 +- pkg/sentry/mm/mm_test.go | 2 +- pkg/sentry/syscalls/linux/sys_rlimit.go | 8 +++++++- test/syscalls/linux/rlimits.cc | 15 +++++++++++---- 7 files changed, 48 insertions(+), 25 deletions(-) diff --git a/pkg/sentry/kernel/fd_map_test.go b/pkg/sentry/kernel/fd_map_test.go index b49996137..9e76f0a2d 100644 --- a/pkg/sentry/kernel/fd_map_test.go +++ b/pkg/sentry/kernel/fd_map_test.go @@ -40,7 +40,7 @@ func newTestFDMap() *FDMap { func TestFDMapMany(t *testing.T) { file := filetest.NewTestFile(t) limitSet := limits.NewLimitSet() - limitSet.Set(limits.NumberOfFiles, limits.Limit{maxFD, maxFD}) + limitSet.Set(limits.NumberOfFiles, limits.Limit{maxFD, maxFD}, true /* privileged */) f := newTestFDMap() for i := 0; i < maxFD; i++ { @@ -64,7 +64,7 @@ func TestFDMapMany(t *testing.T) { func TestFDMap(t *testing.T) { file := filetest.NewTestFile(t) limitSet := limits.NewLimitSet() - limitSet.Set(limits.NumberOfFiles, limits.Limit{1, maxFD}) + limitSet.Set(limits.NumberOfFiles, limits.Limit{1, maxFD}, true /* privileged */) f := newTestFDMap() if _, err := f.NewFDFrom(0, file, FDFlags{}, limitSet); err != nil { @@ -76,7 +76,7 @@ func TestFDMap(t *testing.T) { } largeLimit := limits.Limit{maxFD, maxFD} - limitSet.Set(limits.NumberOfFiles, largeLimit) + limitSet.Set(limits.NumberOfFiles, largeLimit, true /* privileged */) if fd, err := f.NewFDFrom(0, file, FDFlags{}, limitSet); err != nil { t.Fatalf("Adding an FD to a resized map: got %v, want nil", err) @@ -117,7 +117,7 @@ func TestDescriptorFlags(t *testing.T) { file := filetest.NewTestFile(t) f := newTestFDMap() limitSet := limits.NewLimitSet() - limitSet.Set(limits.NumberOfFiles, limits.Limit{maxFD, maxFD}) + limitSet.Set(limits.NumberOfFiles, limits.Limit{maxFD, maxFD}, true /* privileged */) origFlags := FDFlags{CloseOnExec: true} diff --git a/pkg/sentry/limits/limits.go b/pkg/sentry/limits/limits.go index eeca01876..b0571739f 100644 --- a/pkg/sentry/limits/limits.go +++ b/pkg/sentry/limits/limits.go @@ -113,13 +113,17 @@ func (l *LimitSet) SetUnchecked(t LimitType, v Limit) { } // Set assigns value v to resource of LimitType t and returns the old value. -func (l *LimitSet) Set(t LimitType, v Limit) (Limit, error) { +// privileged should be true only when either the caller has CAP_SYS_RESOURCE +// or when creating limits for a new kernel. +func (l *LimitSet) Set(t LimitType, v Limit, privileged bool) (Limit, error) { l.mu.Lock() defer l.mu.Unlock() + // If a limit is already set, make sure the new limit doesn't // exceed the previous max limit. if _, ok := l.data[t]; ok { - if l.data[t].Max < v.Max { + // Unprivileged users can only lower their hard limits. + if l.data[t].Max < v.Max && !privileged { return Limit{}, syscall.EPERM } if v.Cur > v.Max { diff --git a/pkg/sentry/limits/limits_test.go b/pkg/sentry/limits/limits_test.go index d41f62554..945428163 100644 --- a/pkg/sentry/limits/limits_test.go +++ b/pkg/sentry/limits/limits_test.go @@ -20,18 +20,24 @@ import ( ) func TestSet(t *testing.T) { - ls := NewLimitSet() - ls.Set(1, Limit{Cur: 50, Max: 50}) - if _, err := ls.Set(1, Limit{Cur: 20, Max: 50}); err != nil { - t.Fatalf("Tried to lower Limit to valid new value: got %v, wanted nil", err) - } - if _, err := ls.Set(1, Limit{Cur: 20, Max: 60}); err != syscall.EPERM { - t.Fatalf("Tried to raise limit.Max to invalid higher value: got %v, wanted syscall.EPERM", err) - } - if _, err := ls.Set(1, Limit{Cur: 60, Max: 50}); err != syscall.EINVAL { - t.Fatalf("Tried to raise limit.Cur to invalid higher value: got %v, wanted syscall.EINVAL", err) + testCases := []struct { + limit Limit + privileged bool + expectedErr error + }{ + {limit: Limit{Cur: 50, Max: 50}, privileged: false, expectedErr: nil}, + {limit: Limit{Cur: 20, Max: 50}, privileged: false, expectedErr: nil}, + {limit: Limit{Cur: 20, Max: 60}, privileged: false, expectedErr: syscall.EPERM}, + {limit: Limit{Cur: 60, Max: 50}, privileged: false, expectedErr: syscall.EINVAL}, + {limit: Limit{Cur: 11, Max: 10}, privileged: false, expectedErr: syscall.EINVAL}, + {limit: Limit{Cur: 20, Max: 60}, privileged: true, expectedErr: nil}, } - if _, err := ls.Set(1, Limit{Cur: 11, Max: 10}); err != syscall.EINVAL { - t.Fatalf("Tried to set new limit with Cur > Max: got %v, wanted syscall.EINVAL", err) + + ls := NewLimitSet() + for _, tc := range testCases { + if _, err := ls.Set(1, tc.limit, tc.privileged); err != tc.expectedErr { + t.Fatalf("Tried to set Limit to %+v and privilege %t: got %v, wanted %v", tc.limit, tc.privileged, err, tc.expectedErr) + } } + } diff --git a/pkg/sentry/limits/linux.go b/pkg/sentry/limits/linux.go index 295f9c398..e09d0d2fb 100644 --- a/pkg/sentry/limits/linux.go +++ b/pkg/sentry/limits/linux.go @@ -95,6 +95,6 @@ func NewLinuxDistroLimitSet() (*LimitSet, error) { // 1,048,576 ought to be enough for anyone. l := ls.Get(ProcessCount) l.Cur = 1 << 20 - ls.Set(ProcessCount, l) + ls.Set(ProcessCount, l, true /* privileged */) return ls, nil } diff --git a/pkg/sentry/mm/mm_test.go b/pkg/sentry/mm/mm_test.go index e12cb3bd1..ae4fba478 100644 --- a/pkg/sentry/mm/mm_test.go +++ b/pkg/sentry/mm/mm_test.go @@ -70,7 +70,7 @@ func TestUsageASUpdates(t *testing.T) { func TestBrkDataLimitUpdates(t *testing.T) { limitSet := limits.NewLimitSet() - limitSet.Set(limits.Data, limits.Limit{}) // zero RLIMIT_DATA + limitSet.Set(limits.Data, limits.Limit{}, true /* privileged */) // zero RLIMIT_DATA ctx := contexttest.WithLimitSet(contexttest.Context(t), limitSet) mm := testMemoryManager(ctx) diff --git a/pkg/sentry/syscalls/linux/sys_rlimit.go b/pkg/sentry/syscalls/linux/sys_rlimit.go index b0b216045..443334693 100644 --- a/pkg/sentry/syscalls/linux/sys_rlimit.go +++ b/pkg/sentry/syscalls/linux/sys_rlimit.go @@ -106,7 +106,13 @@ func prlimit64(t *kernel.Task, resource limits.LimitType, newLim *limits.Limit) if _, ok := setableLimits[resource]; !ok { return limits.Limit{}, syserror.EPERM } - oldLim, err := t.ThreadGroup().Limits().Set(resource, *newLim) + + // "A privileged process (under Linux: one with the CAP_SYS_RESOURCE + // capability in the initial user namespace) may make arbitrary changes + // to either limit value." + privileged := t.HasCapabilityIn(linux.CAP_SYS_RESOURCE, t.Kernel().RootUserNamespace()) + + oldLim, err := t.ThreadGroup().Limits().Set(resource, *newLim, privileged) if err != nil { return limits.Limit{}, err } diff --git a/test/syscalls/linux/rlimits.cc b/test/syscalls/linux/rlimits.cc index dc31bad9a..5a6174d99 100644 --- a/test/syscalls/linux/rlimits.cc +++ b/test/syscalls/linux/rlimits.cc @@ -25,15 +25,12 @@ namespace { TEST(RlimitTest, SetRlimitHigher) { SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_RESOURCE))); - SKIP_IF(!IsRunningOnGvisor()); struct rlimit rl = {}; EXPECT_THAT(getrlimit(RLIMIT_NOFILE, &rl), SyscallSucceeds()); - // TODO: Even with CAP_SYS_RESOURCE, gVisor does not allow - // setting a higher rlimit. rl.rlim_max++; - EXPECT_THAT(setrlimit(RLIMIT_NOFILE, &rl), SyscallFailsWithErrno(EPERM)); + EXPECT_THAT(setrlimit(RLIMIT_NOFILE, &rl), SyscallSucceeds()); } TEST(RlimitTest, UnprivilegedSetRlimit) { @@ -56,6 +53,16 @@ TEST(RlimitTest, UnprivilegedSetRlimit) { EXPECT_THAT(setrlimit(RLIMIT_NOFILE, &rl), SyscallFailsWithErrno(EPERM)); } +TEST(RlimitTest, SetSoftRlimitAboveHard) { + SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_RESOURCE))); + + struct rlimit rl = {}; + EXPECT_THAT(getrlimit(RLIMIT_NOFILE, &rl), SyscallSucceeds()); + + rl.rlim_cur = rl.rlim_max + 1; + EXPECT_THAT(setrlimit(RLIMIT_NOFILE, &rl), SyscallFailsWithErrno(EINVAL)); +} + } // namespace } // namespace testing -- cgit v1.2.3