Merge ~ddstreet/ubuntu/+source/systemd:revert-sysctl-conf-patch into ~ubuntu-core-dev/ubuntu/+source/systemd:ubuntu-impish

Proposed by Dan Streetman
Status: Merged
Approved by: Lukas Märdian
Approved revision: 5bb4d314bf2f6195d5dbf7ca016b508492c4427c
Merged at revision: 5bb4d314bf2f6195d5dbf7ca016b508492c4427c
Proposed branch: ~ddstreet/ubuntu/+source/systemd:revert-sysctl-conf-patch
Merge into: ~ubuntu-core-dev/ubuntu/+source/systemd:ubuntu-impish
Diff against target: 83 lines (+3/-52)
3 files modified
debian/changelog (+3/-0)
debian/patches/series (+0/-1)
dev/null (+0/-51)
Reviewer Review Type Date Requested Status
Lukas Märdian Approve
Review via email: mp+406474@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Lukas Märdian (slyon) wrote :

Hi Dan! In general this looks good to me and I favor removing delta as much as possible.

Checking the history of this patch, it was introduced in 2017 to set the *.promote_secondaries=1 and *.default_qdisc=fq_codel values, which are the defaults today. (LP: #1721223) – So it's fine keeping those upstream values IMO.

It was then modified in 2019 to explicitly drop upstream's default fs.protected_regular=1 and fs.protected_fifos=1 values. (LP: #1845637) – Checking a current Ubuntu Hirsute system shows:

$ sudo sysctl fs.protected_regular
fs.protected_regular = 2
$ sudo sysctl fs.protected_fifos
fs.protected_fifos = 1

Who sets the fs.protected_regular=2 value (is it the kernel?) – Would upstream systemd's default of fs.protected_regular=1 override this current value, and thus degrate security?

What are your thoughts on this?

Revision history for this message
Dan Streetman (ddstreet) wrote :

> Who sets the fs.protected_regular=2 value

that was set by the procps package, in this commit:
https://salsa.debian.org/debian/procps/-/commit/299f4a1a10810e2995e666374b880b543af8e8e4

for debian bug:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=914859

I removed that file from Ubuntu via bug:
https://bugs.launchpad.net/ubuntu/+source/procps/+bug/1938585

As you can see from the Debian bug, there's no reasoning for setting the default to 2 other than 'probably better', but only protected_regular is increased to 2. I don't have any objection to increasing the protection, but it should happen in upstream systemd, or at least we should have specific reasoning to adjust it in Ubuntu if upstream has reasoning to not increase it (after someone proposes it upstream).

> (is it the kernel?)

the kernel defaults to 0 for both protected_regular and protected_fifo.

Revision history for this message
Lukas Märdian (slyon) wrote :

Thank you Dan for providing those references and background information!

I agree and think we should put the upstream defaults back in place, then.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index 912567b..fde93c5 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -16,6 +16,9 @@ systemd (248.3-1ubuntu4) UNRELEASED; urgency=medium
6 Fix sleep verb used by logind during suspend-then-hibernate
7 (LP: #1934981)
8 https://git.launchpad.net/~ubuntu-core-dev/ubuntu/+source/systemd/commit/?id=b09083d520fecd57ebe28319ad7638442db2e2bd
9+ * d/p/debian/UBUNTU-drop-kernel.-settings-from-sysctl-defaults-shipped.patch:
10+ Revert patch, no reason to diverge from upstream on these sysctl settings
11+ https://git.launchpad.net/~ubuntu-core-dev/ubuntu/+source/systemd/commit/?id=8959a2640c631f026988f1fef6b1a825af6c97dd
12
13 -- Dan Streetman <ddstreet@canonical.com> Tue, 20 Jul 2021 16:53:38 -0400
14
15diff --git a/debian/patches/debian/UBUNTU-drop-kernel.-settings-from-sysctl-defaults-shipped.patch b/debian/patches/debian/UBUNTU-drop-kernel.-settings-from-sysctl-defaults-shipped.patch
16deleted file mode 100644
17index 1284009..0000000
18--- a/debian/patches/debian/UBUNTU-drop-kernel.-settings-from-sysctl-defaults-shipped.patch
19+++ /dev/null
20@@ -1,51 +0,0 @@
21-From: Dimitri John Ledkov <xnox@ubuntu.com>
22-Date: Wed, 11 Oct 2017 12:17:03 +0100
23-Subject: UBUNTU: drop unrelated settings from sysctl defaults shipped by
24- systemd.
25-
26----
27- sysctl.d/50-default.conf | 24 ------------------------
28- 1 file changed, 24 deletions(-)
29-
30-diff --git a/sysctl.d/50-default.conf b/sysctl.d/50-default.conf
31-index f41e24b..87e95d4 100644
32---- a/sysctl.d/50-default.conf
33-+++ b/sysctl.d/50-default.conf
34-@@ -11,26 +11,6 @@
35- # (e.g. /etc/sysctl.d/90-override.conf), and put any assignments
36- # there.
37-
38--# System Request functionality of the kernel (SYNC)
39--#
40--# Use kernel.sysrq = 1 to allow all keys.
41--# See https://www.kernel.org/doc/html/latest/admin-guide/sysrq.html for a list
42--# of values and keys.
43--kernel.sysrq = 16
44--
45--# Append the PID to the core filename
46--kernel.core_uses_pid = 1
47--
48--# Source route verification
49--net.ipv4.conf.default.rp_filter = 2
50--net.ipv4.conf.*.rp_filter = 2
51---net.ipv4.conf.all.rp_filter
52--
53--# Do not accept source routing
54--net.ipv4.conf.default.accept_source_route = 0
55--net.ipv4.conf.*.accept_source_route = 0
56---net.ipv4.conf.all.accept_source_route
57--
58- # Promote secondary addresses when the primary address is removed
59- net.ipv4.conf.default.promote_secondaries = 1
60- net.ipv4.conf.*.promote_secondaries = 1
61-@@ -47,10 +27,6 @@ net.ipv4.conf.*.promote_secondaries = 1
62- # Fair Queue CoDel packet scheduler to fight bufferbloat
63- -net.core.default_qdisc = fq_codel
64-
65--# Enable hard and soft link protection
66--fs.protected_hardlinks = 1
67--fs.protected_symlinks = 1
68--
69- # Enable regular file and FIFO protection
70- fs.protected_regular = 1
71- fs.protected_fifos = 1
72diff --git a/debian/patches/series b/debian/patches/series
73index c0a06a3..b0c5483 100644
74--- a/debian/patches/series
75+++ b/debian/patches/series
76@@ -27,7 +27,6 @@ debian/Revert-pager-stop-disabling-urlification-under-a-pager.patch
77 debian/Ubuntu-UseDomains-by-default.patch
78 debian/Ubuntu-core-in-execute-soft-fail-setting-Nice-priority-when.patch
79 debian/Ubuntu-units-set-ConditionVirtualization-private-users-on-j.patch
80-debian/UBUNTU-drop-kernel.-settings-from-sysctl-defaults-shipped.patch
81 debian/UBUNTU-Add-AssumedApparmorLabel-unconfined-to-timedate1-dbus.patch
82 debian/UBUNTU-test-test-functions-launch-qemu-with-vga-none.patch
83 debian/UBUNTU-wait-online-exit-if-no-links-are-managed.patch

Subscribers

People subscribed via source and target branches