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

Add support for tlp #585

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Add support for tlp #585

wants to merge 11 commits into from

Conversation

barmogund
Copy link

No description provided.


@{bin}/systemctl rix,
@{bin}/logger rix,
@{shells_path} rix,
Copy link
Owner

@roddhjav roddhjav Oct 27, 2024

Choose a reason for hiding this comment

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

That should be @{sh_path} (shell_path is for interactive shell and logging system using that use the user shell).

Comment on lines 50 to 54
owner / r,

owner /etc/tlp.d/ r,
owner /etc/tlp.d/** rw,
owner /etc/udev/udev.conf r,
Copy link
Owner

Choose a reason for hiding this comment

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

Do not use owner in this context. It would prevent any non root user to use the program.

@{bin}/iw rpx,
@{bin}/hdparm rix,
@{bin}/uname rpx,
@{bin}/udevadm rix,
Copy link
Owner

Choose a reason for hiding this comment

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

udevadm should be used in a subprofile: https://apparmor.pujol.io/development/abstractions/#appudevadm
Then, all rule that will go in the subprofile will have to be removed from the main one.

Comment on lines 57 to 58
owner /usr/share/tlp/** rw,
owner /usr/share/tlp/func.d/** rw,
Copy link
Owner

Choose a reason for hiding this comment

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

No owner here.

It is highly unlikely that tlp needs to write something in /usr/share (this folder is considered read-only)


/usr/share/tlp/tlp-readconfs rw,

/var/lib/power-profiles-daemon/{,**} rw,
Copy link
Owner

Choose a reason for hiding this comment

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

This should be restricted: as it write files on another program var, only the specific file names should be allowed and not everything.

/var/lib/power-profiles-daemon/{,**} rw,

owner /usr/share/tlp/bat.d/** rw,
owner /usr/share/perl5/core_perl/** r,
Copy link
Owner

Choose a reason for hiding this comment

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

You don't seems to used perl here, so this should not be here.

If perl is really needed, use the perl abstraction instead.


@{sys}/firmware/acpi/platform_profile* rw,
@{sys}/firmware/acpi/pm_profile* rw,
@{sys}/devices/virtusl/** rw,
Copy link
Owner

Choose a reason for hiding this comment

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

there is a typo: virtual/. Also full write access to it is too wide.

@roddhjav
Copy link
Owner

roddhjav commented Oct 27, 2024

Thanks for this PR. I have added a few minor comments. Once resolved, it will be ready to merge.

You should use the disk-read abstraction, and only add rules on @{sys}/ on system when it is really needed.

Comment on lines 32 to 39
# interaction with tlp
owner @{sys}/devices/@{pci}/ta@{int}/host@{int}/**/**/power/autosuspend_delay_ms r,
owner @{sys}/devices/@{pci}/ta@{int}/host@{int}/scsi_host/host0/link_power_management_policy rw,
owner @{sys}/devices/@{pci}/ta@{int}/host@{int}/target*/**/block/{sda,sr0}/* r,
owner @{sys}/devices/@{pci}/ta@{int}/host@{int}/target*/**/block/{sda,sr0}/dev r,
owner @{sys}/devices/@{pci}/ta@{int}/host@{int}/target*/**/block/sda/sda@{int}/dev r,
owner @{sys}/devices/@{pci}/ta@{int}/host@{int}/target*/**/block/sda@{int}/dev r,

Copy link
Owner

@roddhjav roddhjav Oct 27, 2024

Choose a reason for hiding this comment

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

Most of this should be covered by disk-read.

Note: you cannot have path specific to a given hardware in a rule. Eg: sda sr0, scsi_host, host0. We aim at supporting all distribution and all kind of hardware, therefore, it should also work with sdb, an ssd... Luckily disk-read already handle all of this.

Only add rule here for @{sys} path when writing is needed.

Comment on lines +115 to +116
/dev/disk/by-id/ r,
owner /dev/sda r,
Copy link
Owner

Choose a reason for hiding this comment

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

In disk-read

Comment on lines 72 to 103
@{sys}/bus/ r,
owner @{sys}/bus/pci/drivers/nouveau/ r,
owner @{sys}/devices/@{pci}/ r,
owner @{sys}/devices/@{pci}/power/control rw,
owner @{sys}/devices/@{pci}/ta@{int}/host@{int}/scsi_host/ r,
owner @{sys}/devices/@{pci}/ta@{int}/host@{int}/scsi_host/host@{int}/link_power_management_policy rw,
@{sys}/bus/platform/devices/ r,
@{sys}/class/ r,
@{sys}/class/power_supply/ r,
@{sys}/devices/@{pci}/uevent r,
@{sys}/devices/**/power_supply/*/scope r,
@{sys}/devices/**/power_supply/*/uevent r,
@{sys}/devices/platform/**/uevent r,
@{sys}/devices/system/cpu/*_pstate/{no_turbo,turbo_pct} r,
@{sys}/devices/system/cpu/*_pstate/status r,
@{sys}/devices/system/cpu/cpu@{int}/power/energy_perf_bias rw,
@{sys}/devices/system/cpu/cpufreq/ r,
@{sys}/devices/system/cpu/cpufreq/policy@{int}/energy_performance_preference rw,
@{sys}/devices/system/cpu/cpufreq/policy@{int}/scaling_governor rw,
owner @{sys}/bus/pci/drivers/mei_me/ r,
owner @{sys}/bus/pci/devices/ r,
owner @{sys}/block/ r,
owner @{sys}/class/net/ r,
owner @{sys}/devices/platform/**/**/** r,
owner @{sys}/devices/virtual/block/loop@{int}/ r,
owner @{sys}/devices/virtual/block/loop@{int}/dev r,
owner @{sys}/devices/virtual/net/lo/uevent r,
owner @{sys}/devices/virtual/dmi/id/product_version rw,
owner @{sys}/class/drm/ rw,
owner @{sys}/module/pcie_aspm/parameters/policy rw,
owner @{sys}/module/snd_hda_intel/parameters/power_save rw,
owner @{sys}/module/snd_hda_intel/parameters/power_save_controller rw,
Copy link
Owner

Choose a reason for hiding this comment

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

Use disk-read here too. Only keep what has not been defined in disk-read as well as the few write access.

@{bin}/rm rix,
@{bin}/id rpx,
@{bin}/iw rpx,
@{bin}/hdparm rix,
Copy link
Owner

@roddhjav roddhjav Oct 27, 2024

Choose a reason for hiding this comment

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

Use the profile instead: @{bin}/hdparm rPx,

It should also make a few rule in tlp not needed anymore as they are part of hdparam (possibly all disk related one).

Comment on lines 43 to 44
@{bin}/id rpx,
@{bin}/iw rpx,
Copy link
Owner

Choose a reason for hiding this comment

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

Use rPx instead of rpx.

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.

2 participants