| Message ID | 20201016134835.1886478-1-dburgener@linux.microsoft.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Update SELinuxfs out of tree and then swapover | expand |
On Fri, Oct 16, 2020 at 09:48:31AM -0400, Daniel Burgener wrote: > v2: Include all commits from original series, and include commit ids > > This is a backport for stable of my series to fix a race condition in > selinuxfs during policy load: Has this race condition always been present, or is this a regression that is being fixed from previously working kernels? If it's always been present, why not just use 5.9 to solve it? thanks, greg k-h
On Fri, Oct 16, 2020 at 05:01:20PM +0200, Greg KH wrote: >On Fri, Oct 16, 2020 at 09:48:31AM -0400, Daniel Burgener wrote: >> v2: Include all commits from original series, and include commit ids >> >> This is a backport for stable of my series to fix a race condition in >> selinuxfs during policy load: > >Has this race condition always been present, or is this a regression >that is being fixed from previously working kernels? So this issue has always been there, but: >If it's always been present, why not just use 5.9 to solve it? Because it was merged for 5.10 rather than 5.9, which is a few months out, so Daniel is looking to see if we can have it in 5.8/5.4 to close the gap.
On Fri, Oct 16, 2020 at 11:38:23AM -0400, Sasha Levin wrote: > On Fri, Oct 16, 2020 at 05:01:20PM +0200, Greg KH wrote: > > On Fri, Oct 16, 2020 at 09:48:31AM -0400, Daniel Burgener wrote: > > > v2: Include all commits from original series, and include commit ids > > > > > > This is a backport for stable of my series to fix a race condition in > > > selinuxfs during policy load: > > > > Has this race condition always been present, or is this a regression > > that is being fixed from previously working kernels? > > So this issue has always been there, but: > > > If it's always been present, why not just use 5.9 to solve it? > > Because it was merged for 5.10 rather than 5.9, which is a few months > out, so Daniel is looking to see if we can have it in 5.8/5.4 to close > the gap. I would have to wait for 5.10-rc1 at the earliest, and get the selinux maintainers ack for this :) thanks, greg k-h
On Fri, Oct 16, 2020 at 05:44:43PM +0200, Greg KH wrote: >On Fri, Oct 16, 2020 at 11:38:23AM -0400, Sasha Levin wrote: >> On Fri, Oct 16, 2020 at 05:01:20PM +0200, Greg KH wrote: >> > On Fri, Oct 16, 2020 at 09:48:31AM -0400, Daniel Burgener wrote: >> > > v2: Include all commits from original series, and include commit ids >> > > >> > > This is a backport for stable of my series to fix a race condition in >> > > selinuxfs during policy load: >> > >> > Has this race condition always been present, or is this a regression >> > that is being fixed from previously working kernels? >> >> So this issue has always been there, but: >> >> > If it's always been present, why not just use 5.9 to solve it? >> >> Because it was merged for 5.10 rather than 5.9, which is a few months >> out, so Daniel is looking to see if we can have it in 5.8/5.4 to close >> the gap. > >I would have to wait for 5.10-rc1 at the earliest, and get the selinux >maintainers ack for this :) No objections; if the selinux folks feel unhappy with this it'll just wait for 5.10.
On 10/16/20 11:49 AM, Sasha Levin wrote: > On Fri, Oct 16, 2020 at 05:44:43PM +0200, Greg KH wrote: >> On Fri, Oct 16, 2020 at 11:38:23AM -0400, Sasha Levin wrote: >>> On Fri, Oct 16, 2020 at 05:01:20PM +0200, Greg KH wrote: >>> > On Fri, Oct 16, 2020 at 09:48:31AM -0400, Daniel Burgener wrote: >>> > > v2: Include all commits from original series, and include commit >>> ids >>> > > >>> > > This is a backport for stable of my series to fix a race >>> condition in >>> > > selinuxfs during policy load: >>> > >>> > Has this race condition always been present, or is this a regression >>> > that is being fixed from previously working kernels? >>> >>> So this issue has always been there, but: >>> >>> > If it's always been present, why not just use 5.9 to solve it? >>> >>> Because it was merged for 5.10 rather than 5.9, which is a few months >>> out, so Daniel is looking to see if we can have it in 5.8/5.4 to close >>> the gap. >> >> I would have to wait for 5.10-rc1 at the earliest, and get the selinux >> maintainers ack for this :) > > No objections; if the selinux folks feel unhappy with this it'll just > wait for 5.10. > Yes, that's fine from my end as well. We can carry this series out of tree in the interim. Thanks! -Daniel
On Fri, Oct 16, 2020 at 12:01 PM Daniel Burgener <dburgener@linux.microsoft.com> wrote: > On 10/16/20 11:49 AM, Sasha Levin wrote: > > On Fri, Oct 16, 2020 at 05:44:43PM +0200, Greg KH wrote: > >> On Fri, Oct 16, 2020 at 11:38:23AM -0400, Sasha Levin wrote: > >>> On Fri, Oct 16, 2020 at 05:01:20PM +0200, Greg KH wrote: > >>> > On Fri, Oct 16, 2020 at 09:48:31AM -0400, Daniel Burgener wrote: > >>> > > v2: Include all commits from original series, and include commit > >>> ids > >>> > > > >>> > > This is a backport for stable of my series to fix a race > >>> condition in > >>> > > selinuxfs during policy load: > >>> > > >>> > Has this race condition always been present, or is this a regression > >>> > that is being fixed from previously working kernels? > >>> > >>> So this issue has always been there, but: > >>> > >>> > If it's always been present, why not just use 5.9 to solve it? > >>> > >>> Because it was merged for 5.10 rather than 5.9, which is a few months > >>> out, so Daniel is looking to see if we can have it in 5.8/5.4 to close > >>> the gap. > >> > >> I would have to wait for 5.10-rc1 at the earliest, and get the selinux > >> maintainers ack for this :) > > > > No objections; if the selinux folks feel unhappy with this it'll just > > wait for 5.10. > > > Yes, that's fine from my end as well. We can carry this series out of > tree in the interim. Thanks! I tend to be pretty conservative when it comes to backporting patches to -stable, and since this is both a) big and b) fixes a problem that has existed since the dawn of selinuxfs (and possibly longer <g>) I think the smart thing to do is to wait for v5.10.
v2: Include all commits from original series, and include commit ids This is a backport for stable of my series to fix a race condition in selinuxfs during policy load: selinux: Create function for selinuxfs directory cleanup https://lore.kernel.org/selinux/20200819195935.1720168-2-dburgener@linux.microsoft.com/ selinux: Refactor selinuxfs directory populating functions https://lore.kernel.org/selinux/20200819195935.1720168-3-dburgener@linux.microsoft.com/ selinux: Standardize string literal usage for selinuxfs directory names https://lore.kernel.org/selinux/20200819195935.1720168-4-dburgener@linux.microsoft.com/ selinux: Create new booleans and class dirs out of tree https://lore.kernel.org/selinux/20200819195935.1720168-5-dburgener@linux.microsoft.com/ Several changes were necessary to backport. They are detailed in the commit message for the third commit in the series. The bulk of the original cover letter is reproduced below. In the current implementation, on policy load /sys/fs/selinux is updated by deleting the previous contents of /sys/fs/selinux/{class,booleans} and then recreating them. This means that there is a period of time when the contents of these directories do not exist which can cause race conditions as userspace relies on them for information about the policy. In addition, it means that error recovery in the event of failure is challenging. This patch series follows the design outlined by Al Viro in a previous e-mail to the list[1]. This approach is to first create the new directory structures out of tree, then to perform the swapover, and finally to delete the old directories. Not handled in this series is error recovery in the event of failure. Error recovery in the selinuxfs recreation is unhandled in the current code, so this series will not cause any regression in this regard. Handling directory recreation in this manner is a prerequisite to make proper error handling possible. In order to demonstrate the race condition that this series fixes, you can use the following commands: while true; do cat /sys/fs/selinux/class/service/perms/status >/dev/null; done & while true; do load_policy; done; In the existing code, this will display errors fairly often as the class lookup fails. (In normal operation from systemd, this would result in a permission check which would be allowed or denied based on policy settings around unknown object classes.) After applying this patch series you should expect to no longer see such error messages. Daniel Burgener (4): selinux: Create function for selinuxfs directory cleanup selinux: Refactor selinuxfs directory populating functions selinux: Standardize string literal usage for selinuxfs directory names selinux: Create new booleans and class dirs out of tree security/selinux/selinuxfs.c | 168 +++++++++++++++++++++++++++-------- 1 file changed, 129 insertions(+), 39 deletions(-)