Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify SELinux context of MPS pipe directory #789

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

elezar
Copy link
Member

@elezar elezar commented Jun 27, 2024

No description provided.

@elezar elezar requested a review from cdesiniotis June 27, 2024 09:19
@elezar elezar self-assigned this Jun 27, 2024
@elezar
Copy link
Member Author

elezar commented Jun 27, 2024

/cc @empovit

Comment on lines +101 to +105
if selinux.EnforceMode() == selinux.Enforcing {
if err := selinux.Chcon(pipeDir, "container_file_t", true); err != nil {
return fmt.Errorf("error setting SELinux context: %w", err)
}
}
Copy link

Choose a reason for hiding this comment

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

It's a one-time initialization step, isn't it? I'm wondering if it's possible that a user sets SELinux to permissive (e.g. for debugging), installs the driver, and then changes SELinux back to enforcing. Won't it be safer to rely on selinux.GetEnabled() or selinux.DefaultEnforceMode()?

Copy link

Choose a reason for hiding this comment

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

I've just tested chcon on a system with SELinux disabled, and it works. I guess it won't have any effect, but won't fail either.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy to switch to GetEnabled() if we think that's cleaner.

It's a one-time initialization step, isn't it?

This step is only run as the MPS daemon container is started -- or if the configuration changes. What is the expected behaviour if a user changes the SELinux mode?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to clarify, EnforceMode contains the logic to extract the CURRENT mode.

// enforceMode returns the current SELinux mode Enforcing, Permissive, Disabled
func enforceMode() int {
	var enforce int

	enforceB, err := os.ReadFile(selinuxEnforcePath())
	if err != nil {
		return -1
	}
	enforce, err = strconv.Atoi(string(enforceB))
	if err != nil {
		return -1
	}
	return enforce
}

Whereas GetEnabled() checks whether SELinux is available at all -- regardless of the enforcing mode.

Copy link

Choose a reason for hiding this comment

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

Right, so in the enforcing mode the new type will do the tick, in the permissive mode everything is permitted anyway, so changing the type won't have any effect. Unless the mode is switched back to enforcing, in which case the type will already be set to the right one.

The only reason we may need GetEnable() is to avoid errors when trying to change the type on a system that doesn't run SELinux, IMO.

Copy link

Choose a reason for hiding this comment

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

Actually, it looks like enabling/disabling SELinux or changing the mode (enforcing/permissive) requires rebooting the node anyway. So the type will be applied whenever needed in any case.

@empovit
Copy link

empovit commented Jun 27, 2024

LGTM

Signed-off-by: Evan Lezar <elezar@nvidia.com>
@elezar elezar merged commit 6c5fa87 into NVIDIA:main Jul 10, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants