interfaces: add hwmon-control interface for hwmon channels

[digitalrane]

This interface is designed to allow controlling hardware and platform devices via sysfs nodes. The example which necessitated the creation on this interface is enabling control of PWM devices (chassis fans, for example) which requires write access to `

An example, with a snap which is trying to enable PWM controls, installed in --devmode (hence why these are ALLOWED rather than DENIED messages:

AVC apparmor="ALLOWED" operation="open" profile="snap.lm-sensors.fancontrol" name="/sys/devices/platform/nct6775.656/hwmon/hwmon1/pwm2_enable" pid=744 comm="fancontrol" requested_mask="wc" denied_mask="wc" fsuid=0 ouid=0

I have supplied unit tests for the new interface but have not updated the documentation - any guidance on how to best document this interface, should it be merged, would be appreciated.

I have used the sysfs apparmor rule from the hardware-observe interface, which this interface is meant to be used in conjunction to allow control of devices rather than simply read access. I have removed all original rules, including the seccomp policy as I believe the only use case for hardware-control would at least initially rely on adding write support to device nodes themselves - adding write access to the other devices, proc entries and configuration files - as well as the capabilities being allowed by the hardware-observe interfaces seemed like definite overkill.

I suspect this interface would also then need to be added to the core snap so that the plug could be connected to system, is this correct? Guidance here would also be appreciated.

Signed-off-by: James Hebden [email protected]

[digitalrane]

Hey.

Thank you for submitting this. I did a quick review and asked some questions inline.

Thank you for the review - the feedback is appreciated. I think I've addressed the issues you had highlighted - please let me know if further changes are required based on my responses to your other feedback.

[pedronis]

We have already some *-control interfaces that grant specific write access to subtrees of /sys/. In general I think we are more comfortable having more device/device-class specific interfaces than a very general one. Could some access be added to some of the preexisting interfaces that makes sense? or a more specific new one be introduced?

In general we are not trying to minimize the number of interfaces having just a few powerful ones. we care more about the intention and the consequences of the interfaces to be clear.

Would the snap using this live in the main store?

[digitalrane]

Thanks for that insight into the current thinking on interfaces. Specifically, the utility of the interface for me currently was for granting access to hwmon-class devices, specifically the control capabilities accessed via sysfs, for a snap that is indeed intended for the public store [0].

Given the read of these attributes and sysfs nodes is being allowed via the hardware-observe interface, it felt more ergonomic to me to have this information writable via a hardware-control interface, however I don't have a strong preference for this approach, and indeed was trying to prevent interface-sprawl.

I would be happy to rework this PR into a 'hwmon-control' interface if that would be more suitable. If there's appetite for that, let me know, I'll update the PR.

[0] https://git.ec0.io/ec0/lm-sensors-snap

[pedronis]

I think a more specific interface is preferable, maybe @jdstrand has a different opinion though

[pedronis]

marking blocked because it needs to be reformulated as a more precise interface or adding the access to relevant preexisting interfaces

[jdstrand]

I think a more specific interface is preferable, maybe @jdstrand has a different opinion though

I agree that hardware-control is too general. Before making changes to the PR, can you put your snap in devmode, then connect all the interfaces that you need (adding hardware-observe since it has 'r' access for your /sys accesses), then exercise your snap and paste security policy violations from your snap?

This will give us both a better idea of your specific needs and a fuller understanding of the context for organizing these accesses.

[digitalrane]

Hey @jdstand - I had previously done that, and the only deny I was getting was the AppArmor deny I listed in the opening comment of the issue. Everything else is a read which is covered by the hardware-observe, spi, i2c and gpio interfaces.

I can do a full profile at some point tonight, but it seems more and more like refactoring this as a hwmon-focused interface seems like the way to proceed.

[jdstrand]

After reading https://www.kernel.org/doc/Documentation/hwmon/sysfs-interface I think I have come up with a plan. In essence, hwmon is exposed in regular places in sysfs and the upstream documentation describes the various types of devices (or hwmon 'channels') that are supported. For the most part, the files are regularly named. Since all of this falls under the 'hwmon' banner, I suggest a new 'hwmon-control' interface which accepts an optional 'channels' attribute. Like so:

plugs:
  hwmon-control:
    channels:
    - fan
    - temperature
    - ...

If 'channels' is omitted, all the channels are allowed. In this manner, a generic lm-sensors snap might use this to access everything:

apps:
  lm-sensors:
    plugs:
    - hwmon-control

but a snap that only needs to look at the fans uses:

plugs:
  hwmon-pwm:
    interface: hwmon-control
    channels:
    - pwm
apps:
  foo:
    plugs:
    - hwmon-pwm

The interface would be manually connected. Snap declarations could be used to auto-connect based on interface attributes (or not).

Looking at the upstream documentation, I came up with the following (recall that in the below policy these are not regular expressions, but AARE):

• Common policy that is added only once when plugging hwmon-control, regardless of the number of attributes:

# https://www.kernel.org/doc/Documentation/hwmon/sysfs-interface
# hwmon - common accesses
/run/udev/data/+hwmon:hwmon[0-9]* r,
/sys/class/hwmon/ r,
/sys/devices/**/hwmon[0-9]*/ r,
/sys/devices/**/hwmon[0-9]*/name r,
/sys/devices/**/hwmon[0-9]*/update_interval r,
#/sys/devices/**/hwmon[0-9]*/update_interval w, # snapd needs dynamic detection to mediate writes

# alarms
/sys/devices/**/hwmon[0-9]*/beep_enable r,
#/sys/devices/**/hwmon[0-9]*/beep_enable w, # snapd needs dynamic detection to mediate writes

# Deprecated for old drivers using a non-standard interface to alarms
# and beeps.
#/sys/devices/**/hwmon[0-9]*/alarms r,
#/sys/devices/**/hwmon[0-9]*/beep_mask r,
#/sys/devices/**/hwmon[0-9]*/beep_mask rw, # snapd needs dynamic detection to mediate writes

• voltage:

hwmon-control:
 channel:
 - voltage

# hwmon - voltage
/sys/devices/**/hwmon[0-9]*/in[0-9]*_min rw,
/sys/devices/**/hwmon[0-9]*/in[0-9]*_max rw,
/sys/devices/**/hwmon[0-9]*/in[0-9]*_crit rw,
/sys/devices/**/hwmon[0-9]*/in[0-9]*_lcrit rw,
/sys/devices/**/hwmon[0-9]*/in[0-9]*_input r,
/sys/devices/**/hwmon[0-9]*/in[0-9]*_average r,
/sys/devices/**/hwmon[0-9]*/in[0-9]*_lowest r,
/sys/devices/**/hwmon[0-9]*/in[0-9]*_highest r,
/sys/devices/**/hwmon[0-9]*/in[0-9]*_reset_history w,
/sys/devices/**/hwmon[0-9]*/in_reset_history w,
/sys/devices/**/hwmon[0-9]*/in[0-9]*_label r,
/sys/devices/**/hwmon[0-9]*/in[0-9]*_enable rw,
/sys/devices/**/hwmon[0-9]*/cpu[0-9]*_vid r,
/sys/devices/**/hwmon[0-9]*/vrm rw,
# alarms
/sys/devices/**/hwmon[0-9]*/in[0-9]*_alarm r,
/sys/devices/**/hwmon[0-9]*/in[0-9]*_min_alarm r,
/sys/devices/**/hwmon[0-9]*/in[0-9]*_max_alarm r,
/sys/devices/**/hwmon[0-9]*/in[0-9]*_lcrit_alarm r,
/sys/devices/**/hwmon[0-9]*/in[0-9]*_crit_alarm r,
/sys/devices/**/hwmon[0-9]*/in[0-9]*_beep rw,

• fan:

hwmon-control:
 channel:
 - fan

# hwmon - fan
/sys/devices/**/hwmon[0-9]*/fan[1-9]*_min rw,
/sys/devices/**/hwmon[0-9]*/fan[1-9]*_max rw,
/sys/devices/**/hwmon[0-9]*/fan[1-9]*_input r,
/sys/devices/**/hwmon[0-9]*/fan[1-9]*_div rw,
/sys/devices/**/hwmon[0-9]*/fan[1-9]*_pulses	rw,
/sys/devices/**/hwmon[0-9]*/fan[1-9]*_target rw,
/sys/devices/**/hwmon[0-9]*/fan[1-9]*_label r,
/sys/devices/**/hwmon[0-9]*/fan[1-9]*_enable rw,
# alarms
/sys/devices/**/hwmon[0-9]*/fan[1-9]*_alarm r,
/sys/devices/**/hwmon[0-9]*/fan[1-9]*_min_alarm r,
/sys/devices/**/hwmon[0-9]*/fan[1-9]*_max_alarm r,
/sys/devices/**/hwmon[0-9]*/fan[1-9]*_beep rw,
# faults
/sys/devices/**/hwmon[0-9]*/fan[1-9]*_fault r,

• pwm:

hwmon-control:
 channel:
 - pwm

# hwmon - pwm
/sys/devices/**/hwmon[0-9]*/pwm[1-9]* rw,
/sys/devices/**/hwmon[0-9]*/pwm[1-9]*_enable rw,
/sys/devices/**/hwmon[0-9]*/pwm[1-9]*_mode rw,
/sys/devices/**/hwmon[0-9]*/pwm[1-9]*_freq rw,
/sys/devices/**/hwmon[0-9]*/pwm[1-9]*_auto_channels_temp rw,
/sys/devices/**/hwmon[0-9]*/pwm[1-9]*_auto_point[1-9]*_pwm rw,
/sys/devices/**/hwmon[0-9]*/pwm[1-9]*_auto_point[1-9]*_temp rw,
/sys/devices/**/hwmon[0-9]*/pwm[1-9]*_auto_point[1-9]*_temp_hyst rw,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_auto_point[1-9]*_pwm rw,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_auto_point[1-9]*_temp rw,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_auto_point[1-9]*_temp_hyst rw,

• temperature:

hwmon-control:
 channel:
 - temperature

# hwmon - temperature
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_type rw,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_max rw,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_min rw,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_max_hyst rw,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_min_hyst rw,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_input r,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_crit rw,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_crit_hyst rw,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_emergency rw,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_emergency_hyst rw,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_lcrit rw,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_lcrit_hyst rw,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_offset rw,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_label r,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_lowest r,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_highest r,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_reset_history w,
/sys/devices/**/hwmon[0-9]*/temp_reset_history w,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_enable rw,
# alarms
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_alarm r,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_min_alarm r,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_max_alarm r,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_lcrit_alarm r,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_crit_alarm r,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_emergency_alarm r,
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_beep rw,
# faults
/sys/devices/**/hwmon[0-9]*/temp[1-9]*_fault r,

• current:

hwmon-control:
 channel:
 - current

# hwmon - current
/sys/devices/**/hwmon[0-9]*/curr[1-9]*_max rw,
/sys/devices/**/hwmon[0-9]*/curr[1-9]*_min rw,
/sys/devices/**/hwmon[0-9]*/curr[1-9]*_lcrit rw,
/sys/devices/**/hwmon[0-9]*/curr[1-9]*_crit rw,
/sys/devices/**/hwmon[0-9]*/curr[1-9]*_input r,
/sys/devices/**/hwmon[0-9]*/curr[1-9]*_average r,
/sys/devices/**/hwmon[0-9]*/curr[1-9]*_lowest r,
/sys/devices/**/hwmon[0-9]*/curr[1-9]*_highest r,
/sys/devices/**/hwmon[0-9]*/curr[1-9]*_reset_history w,
/sys/devices/**/hwmon[0-9]*/curr_reset_history w,
/sys/devices/**/hwmon[0-9]*/curr[1-9]*_enable rw,
# alarms
/sys/devices/**/hwmon[0-9]*/curr[1-9]*_alarm r,
/sys/devices/**/hwmon[0-9]*/curr[1-9]*_min_alarm r,
/sys/devices/**/hwmon[0-9]*/curr[1-9]*_max_alarm r,
/sys/devices/**/hwmon[0-9]*/curr[1-9]*_lcrit_alarm r,
/sys/devices/**/hwmon[0-9]*/curr[1-9]*_crit_alarm r,
/sys/devices/**/hwmon[0-9]*/curr[1-9]*_beep rw,

• power:

hwmon-control:
 channel:
 - power

# hwmon - power
/sys/devices/**/hwmon[0-9]*/power[1-9]*_average r,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_average_interval rw,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_average_interval_max r,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_average_interval_min r,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_average_highest r,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_average_lowest r,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_average_max rw,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_average_min rw,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_input r,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_input_highest r,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_input_lowest r,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_reset_history w,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_accuracy r,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_cap rw,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_cap_hyst rw,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_cap_max r,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_cap_min r,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_max rw,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_crit rw,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_enable rw,
# alarms
/sys/devices/**/hwmon[0-9]*/power[1-9]*_alarm r,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_cap_alarm r,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_max_alarm r,
/sys/devices/**/hwmon[0-9]*/power[1-9]*_crit_alarm r,

• energy:

hwmon-control:
 channel:
 - energy

# hwmon - energy
/sys/devices/**/hwmon[0-9]*/energy[1-9]*_input r,
/sys/devices/**/hwmon[0-9]*/energy[1-9]*_enable rw,

• humidity:

hwmon-control:
 channel:
 - humidity

# hwmon - humidity
/sys/devices/**/hwmon[0-9]*/humidity[1-9]*_input r,
/sys/devices/**/hwmon[0-9]*/humidity[1-9]*_enable rw,

• intrusion:

hwmon-control:
 channel:
 - intrusion

# hwmon - intrusion
/sys/devices/**/hwmon[0-9]*/intrusion[0-9]*_alarm rw,
/sys/devices/**/hwmon[0-9]*/intrusion[0-9]*_beep rw,

@devec0 - I understand implementing the above is more than what you signed up for, and this is something that I can implement if you prefer. Can you at least verify that the above would handle your use case?

@pedronis - can you comment on the name of the interface and the approach?

[digitalrane]

Hey @jdstrand - thanks for the thorough review and all the time you've put into accommodating this MP. I've looked over the proposed patterns and think they would cover the use case well - and I like the channel idea for making the interface more specific. I've already made a start at implementing this change, so am happy to take care of revising the MP. I should have something over the next few days hopefully.

One question I had - (for anyone who has written tests for snapd, really) - previously @zyga had mentioned that the test suite provided did not exercise the interface fully. This raised a good point, and something I noticed when reviewing snapd tests for examples initially - a number of the tests in the snapd codebase do not really do this (hardware-monitor being one of them, which my original hardware-control MP was based on), I assume due to the challenges around mocking this file access and/or the breadth of some of the interfaces. This is not to criticise the testing or quality of snapd, but rather to raise this testing challenge as a topic of discussion.

I think the challenge of fully exercising interfaces is pretty clear - without some level of mocking in place, given the system/OS-oriented nature of interfaces and the disparity between end user and CI/CD systems from a hardware and kernel perspective - the diversity of environments and possible configurations are large, and the nature of the access is very low-level, so fully exercising interfaces via test cases is a fairly unique challenge for snapd.

I propose handling this (for this MP) by having a test snap with several iterations of the plug in the snap, each connected to an app which uses a different channel parameter, also passing a parameter to the test app inside the snap, which will exercise all of the file patterns we are controlling with this interface. Given the specific unlikelihood of hwmon devices existing on CI/CD systems, and the general anti-pattern of relying on device existence during testing - I would propose that the best solution to the pass/fail criteria would be to test dummy device names for -EACCESS vs -ENOENT to see if the interface is correctly allowing or denying access to files based on connected interfaces. Does this approach sound acceptable?

The only other solution which comes to mind would be implementing a level of mocking/patching of file access to guarentee consistent file "availability", which would become quite complex for the snap-based tests.

[pedronis]

@jdstrand

@pedronis - can you comment on the name of the interface and the approach?

the interface name (use the a name based on the sysfs naming as for other interfaces we already have) and the approach seems alright, it's a bit unfortunate that this has channels that means a very different thing but it's kernel terminology afaict

[jdstrand]

@pedronis - can you comment on the name of the interface and the approach?

the interface name (use the a name based on the sysfs naming as for other interfaces we already have) and the approach seems alright, it's a bit unfortunate that this has channels that means a very different thing but it's kernel terminology afaict

@devec0 - please feel free to continue as I laid out. If you need me to pick it up, let me know. Thanks!

[jdstrand]

@devec0 - were you able to move forward on this? Should someone else pick it up?

[jdstrand]

I propose handling this (for this MP) by having a test snap with several iterations of the plug in the snap, each connected to an app which uses a different channel parameter, also passing a parameter to the test app inside the snap, which will exercise all of the file patterns we are controlling with this interface. Given the specific unlikelihood of hwmon devices existing on CI/CD systems, and the general anti-pattern of relying on device existence during testing - I would propose that the best solution to the pass/fail criteria would be to test dummy device names for -EACCESS vs -ENOENT to see if the interface is correctly allowing or denying access to files based on connected interfaces. Does this approach sound acceptable?

There are functional tests in tests/ (driven by spread) for this sort of thing. @sergiocazzolato has worked a lot with functional testing of the interfaces and may be able to provide some guidance (or we can commit the implementation we have and have spread tests come in a subsequent PR).

[digitalrane]

@devec0 - were you able to move forward on this? Should someone else pick it up?

Hey, I'm definitely still planning on moving this forward, was just moving house the last few weeks. Should have some time to work on it over the next week or so. Thanks for all the feedback on the merge so far!

[anonymouse64]

@devec0 did you re-implement this interface to match Jamie's suggestion? If so I think this needs a master merge and it can be unblocked.

[mardy]

@devec0 did you re-implement this interface to match Jamie's suggestion? If so I think this needs a master merge and it can be unblocked.

@devec0 also, if you do not think you can find the time or the energy for this, please let us know so that someone (me?) can "steal" your branch and push this forward. :-)

[digitalrane]

@devec0 did you re-implement this interface to match Jamie's suggestion? If so I think this needs a master merge and it can be unblocked.

@devec0 also, if you do not think you can find the time or the energy for this, please let us know so that someone (me?) can "steal" your branch and push this forward. :-)

Hey @mardy - absolutely no problems from my side. It would be great to see it merged, it's just been so long since I've used telegraf that I haven't been able to justify putting the time into it. Glad someone might benefit from all of that work afterall 😄

[digitalrane]

I've added you as a collaborator on my fork @mardy - so you can commit changes without losing all of the history in this PR. Let me know if you have any issues.

[mardy]

I've added you as a collaborator on my fork @mardy - so you can commit changes without losing all of the history in this PR. Let me know if you have any issues.

Thanks a lot! I'll try to find some time for this. :-)

[mardy]

Hi James (@devec0), I pushed an update to this. Only spread tests are missing, but I'll work on them.

I accidentally pushed a branch hwmon-control-interface, identical to this one, onto your snapd repo, sorry. Feel free to delete it, if it bothers you. :-)

[mardy]

This should be now ready for review. I tested it with a lm-sensors snap, which seems to work fine: I get the same output as the deb version, except for a few localization issues like missing degree sign on temperature values.

However, a few denials are printed in the audit log, like

AVC apparmor="DENIED" operation="open" profile="snap.lm-sensors.sensors" name="/sys/devices/pci0000:00/0000:00:03.1/0000:09:00.0/i2c-3/name" pid=166457 comm="sensors" requested_mask="r" denied_mask="r" fsuid=1000 ouid=0
AVC apparmor="DENIED" operation="open" profile="snap.lm-sensors.sensors" name="/sys/devices/pci0000:00/0000:00:14.0/i2c-1/name" pid=166457 comm="sensors" requested_mask="r" denied_mask="r" fsuid=1000 ouid=0

If my understanding is right, it's because lm-sensors also supports kernels without the hwmon device class, and tries to directly access the physical devices. I wouldn't bother suppressing these.

[codecov-commenter]

Codecov Report

Merging #8592 (5e5ca97) into master (34b32c2) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #8592      +/-   ##
==========================================
+ Coverage   78.35%   78.37%   +0.01%     
==========================================
  Files         921      922       +1     
  Lines      105004   105081      +77     
==========================================
+ Hits        82280    82357      +77     
+ Misses      17599    17598       -1     
- Partials     5125     5126       +1     

Flag	Coverage Δ
unittests	78.37% <100.00%> (+0.01%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
interfaces/builtin/hwmon_control.go	100.00% <100.00%> (ø)
osutil/synctree.go	76.41% <0.00%> (-2.84%)	:arrow_down:
store/cache.go	69.23% <0.00%> (-1.93%)	:arrow_down:
overlord/snapstate/snapstate.go	83.29% <0.00%> (+0.13%)	:arrow_up:
overlord/devicestate/devicestate.go	78.70% <0.00%> (+0.15%)	:arrow_up:
overlord/ifacestate/helpers.go	77.45% <0.00%> (+0.48%)	:arrow_up:
cmd/snap/cmd_aliases.go	91.54% <0.00%> (+1.40%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 34b32c2...5e5ca97. Read the comment docs.

[Meulengracht]

@digitalrayne is this still relevant? otherwise i'll close this

[Meulengracht]

I am closing this - but feel free to reopen should it be relevant again