Skip to content

Conversation

Fil0sOFF
Copy link

@Fil0sOFF Fil0sOFF commented Aug 24, 2025

Description

fancontrol.service was "hardened" in Debian trixie. See Debian bug #944808.
fancontrol can't access fans in /dev after that.
This change returns full access to /dev for fancontrol.service

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings

fancontrol.service was "hardened" in Debian trixie. See Debian bug #944808.
fancontrol can't access fans in /dev after this.
This change returns full access to /dev for fancontrol.service
@Fil0sOFF Fil0sOFF requested review from prahal and a team as code owners August 24, 2025 18:22
@github-actions github-actions bot added size/small PR with less then 50 lines 08 Milestone: Third quarter release Needs review Seeking for review Hardware Hardware related like kernel, U-Boot, ... BSP Board Support Packages labels Aug 24, 2025
Copy link
Contributor

coderabbitai bot commented Aug 24, 2025

Walkthrough

Adds a new systemd drop-in override for fancontrol on Helios64 to disable PrivateDevices. Introduces packages/bsp/helios64/fancontrol.service.noprivdev-override with [Service] PrivateDevices=no. Updates config/boards/helios64.conf to install this override to /etc/systemd/system/fancontrol.service.d/noprivdev.conf (mode 0644) during post_family_tweaks_bsp__helios64, positioned after the existing fancontrol.service.pid-override installation. No other changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

Ready to merge, 05

Suggested reviewers

  • Heisath

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@coderabbitai coderabbitai bot requested a review from Heisath August 24, 2025 18:24
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (4)
packages/bsp/helios64/fancontrol.service.noprivdev-override (2)

1-2: Retain hardening where possible: consider binding only the needed /dev nodes instead of full de-sandboxing

PrivateDevices=no removes the /dev sandbox entirely. If the board only needs specific device nodes (e.g., /dev/i2c-0,/dev/i2c-1), a tighter alternative is to keep PrivateDevices=yes and bind just those nodes. That preserves most of the defense-in-depth.

Example alternative drop-in (adjust devices to what Helios64 actually uses):

[Service]
PrivateDevices=yes
BindPaths=/dev/i2c-0:/dev/i2c-0
BindPaths=/dev/i2c-1:/dev/i2c-1

If PWM is only via sysfs (/sys/class/hwmon or /sys/class/pwm), PrivateDevices should not matter; in that case, re-check what exactly requires /dev on Helios64 (e.g., an I²C fan controller).

Would you like me to open a follow-up to prototype a minimal-bind setup once we confirm the exact device nodes needed?


1-2: Add a short rationale comment for future maintainers

Add context so this override isn’t “mysterious” a year from now. Suggestion:

-[Service]
-PrivateDevices=no
+# Helios64: Debian trixie fancontrol hardening (see Debian#944808) isolates /dev and breaks access
+# to required device nodes on this board. Restore access here; consider minimal BindPaths in future.
+[Service]
+PrivateDevices=no

I can amend the PR with this comment if you agree.

config/boards/helios64.conf (2)

43-43: Install path is fine; consider quoting vars and deterministic drop-in ordering

  • Quoting $SRC and $destination is a minor hardening against whitespace/expansion issues.
  • Using a numeric prefix like 10-noprivdev.conf creates deterministic ordering if more drop-ins are added later (even though there’s no conflict today).

Proposed tweak:

-install -m 644 $SRC/packages/bsp/helios64/fancontrol.service.noprivdev-override $destination/etc/systemd/system/fancontrol.service.d/noprivdev.conf
+install -m 0644 "$SRC/packages/bsp/helios64/fancontrol.service.noprivdev-override" "$destination/etc/systemd/system/fancontrol.service.d/10-noprivdev.conf"

43-43: Ensure the drop-in is picked up on upgrades without requiring a reboot

If this file lands via a BSP package update on an already running system, systemd won’t notice new drop-ins until a daemon-reload (and fancontrol won’t pick it up until restart). Consider adding a daemon-reload + try-restart in the BSP postinst path.

Reference (from lib/functions/bsp/armbian-bsp-cli-deb.sh, board_side_bsp_cli_postinst_finish): it currently enables several services with --no-reload. You could append:

systemctl daemon-reload || true
systemctl try-restart fancontrol.service || true

This can be a separate, tiny follow-up PR if you prefer to keep this one scoped.

I can draft that follow-up change against armbian-bsp-cli postinst if you want.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 5eb8bd0 and dbb4a66.

📒 Files selected for processing (2)
  • config/boards/helios64.conf (1 hunks)
  • packages/bsp/helios64/fancontrol.service.noprivdev-override (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
config/boards/helios64.conf (1)
lib/functions/bsp/armbian-bsp-cli-deb.sh (2)
  • board_side_bsp_cli_postinst_finish (448-471)
  • board_side_bsp_cli_postrm (417-423)
🔇 Additional comments (1)
packages/bsp/helios64/fancontrol.service.noprivdev-override (1)

1-2: Functional fix: disabling PrivateDevices restores /dev access for fancontrol on trixie

This is a pragmatic drop-in that should immediately unblock Helios64 on Debian trixie where the hardened unit breaks access to device nodes. No syntax issues. Applies cleanly as a unit drop-in.

Copy link
Collaborator

@leggewie leggewie left a comment

Choose a reason for hiding this comment

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

am I correct to think that you are overriding the hardening feature? that sounds like a bad idea.

PrivateDevices was specifically and explicitly set to yes in the bug ticket you mentioned

@Fil0sOFF
Copy link
Author

A more granular approach would be to allow access only to required devices. I.e.

[Service]
BindPaths=/dev/fan-p6
BindPaths=/dev/fan-p7
BindPaths=/dev/thermal-cpu

Does that look better?
I can update the PR.

@leggewie
Copy link
Collaborator

Certainly better, but I am not sure it addresses the issue properly. I haven't had the chance to look into it deeply.

You are circumventing an explicitly set security-policy. Just doesn't sound right.

@Fil0sOFF
Copy link
Author

As far as i understand, 90-helios64-hwmon.rules in helios64 bsp symlinks path to fan and thermal controls into /dev/

user@helios64:~$ ls -l /dev/{fan,thermal}*
lrwxrwxrwx 1 root root 41 Aug 24 22:11 /dev/fan-p6 -> /sys/devices/platform/p6-fan/hwmon/hwmon6
lrwxrwxrwx 1 root root 41 Aug 24 22:11 /dev/fan-p7 -> /sys/devices/platform/p7-fan/hwmon/hwmon5
lrwxrwxrwx 1 root root 60 Aug 24 22:11 /dev/thermal-board -> /sys/devices/platform/ff120000.i2c/i2c-2/2-004c/hwmon/hwmon4
lrwxrwxrwx 1 root root 49 Aug 24 22:11 /dev/thermal-cpu -> /sys/devices/virtual/thermal/thermal_zone0/hwmon0

I guess those udev rules must be used to deal with devices inside /sys/devices changing paths sometimes.
It may be possible that those symlinks are not needed at all and we could use /sys/devices/ paths in /etc/fancontrol. But someone needs to confirm that.

Also, helios4 bsp has similar udev rules, so it must be suffering from this issue as well.

@leggewie
Copy link
Collaborator

Also, helios4 bsp has similar udev rules, so it must be suffering from this issue as well.

I see. I have a helios4 unit but won't have access to it for another 2 months.

@leggewie
Copy link
Collaborator

leggewie commented Sep 3, 2025

My reading of the PrivateDevices= section at https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html is that the correct thing to do is to set DeviceAllow=

What we probably need is a /etc/systemd/system/fancontrol.service file

[Unit]
Description=Fan Control Service

[Service]
Type=simple
ExecStart=/usr/sbin/fancontrol
DeviceAllow=/dev/fan rw
DeviceAllow=/dev/sensors rw

[Install]
WantedBy=multi-user.target

@Fil0sOFF
Copy link
Author

That was my initial thought as well. But it doesn't work. I guess it works for only device files, not directories.BindPaths method works fine though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
08 Milestone: Third quarter release BSP Board Support Packages Hardware Hardware related like kernel, U-Boot, ... Needs review Seeking for review size/small PR with less then 50 lines
Development

Successfully merging this pull request may close these issues.

2 participants