linux-kernel - Re: [RFC PATCH 0/2] Sensor readings fixes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <faf91200-4790-1210-7ba5-7892c98fcb5e@gmail.com>
Date:   Fri, 25 Mar 2022 14:38:06 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Cristian Marussi <cristian.marussi@....com>,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Cc:     sudeep.holla@....com, souvik.chakravarty@....com,
        nicola.mazzucato@....com
Subject: Re: [RFC PATCH 0/2] Sensor readings fixes

On 3/25/22 04:41, Cristian Marussi wrote:
> On Mon, Dec 20, 2021 at 05:41:53PM +0000, Cristian Marussi wrote:
>> Hi,
>>
>> this was supposed to be an easy fix on how sensor readings are handled
>> across different FW versions while maintaining backward compatibility,
>> but the solution raised for me more questions than the issue itself...
>> ...so I posted as an RFC.
>>
>> In a nutshell, since SCMI FWv3.0 spec, sensors SCMI_READING_GET command
>> can report axis and timestamps too, beside readings, so a brand new
>> scmi_reading_get_timestamped protocol operation was exposed (used by IIO)
>> while the old scmi_reading_get was kept as it was, already used by HWMON
>> subsystem for other classes of sensors.
>>
>> Unfortunately, also the flavour of reported values changed from unsigned
>> to signed with v3.0, so if you end-up on a system running against an SCMI
>> v3.0 FW platform you could end up reading a negative value and interpreting
>> it as a big positive since scmi_reading_get reports only u64.
>>
>> 01/02 simply takes care, when a FW >= 3.0 is detected, to return an error
>> to any scmi_reading_get request if that would result in tryinh to carry
>> a negative value into an u64.
>>
>> So this should rectify the API exposed by SCMI sensor and make it
>> consistent in general, in such a way that a user calling it won't risk to
>> receive a false big-positive which was indeed a 2-complement negative from
>> the perpective of the SCMI fw.
>> 	
>> So far so good...sort of...since, to make things more dire, the HWMON
>> interface, which is the only current upstream user of scmi_reading_get
>> DOES allow indeed to report to the HWMON core negative values, so it was
>> just that we were silently interpreting u64 as s64 :P ...
>>
>> ...as a consequence the fix above to the SCMI API will potentially break
>> this undocumented behaviour of our only scmi_reading_get user.
>>
>> Additionally, while looking at this, I realized that for similar reasons
>> even on systems running the current SCMI stack API and an old FW <=2.0
>> the current HWMON read is potentially broken, since when the FW reports
>> a very big and real positive number we'll report it as a signed long to
>> the HWMON core, so turning it wrongly into a negative report: for this
>> reason 02/02 adds a check inside scmi-hwmon to filter out, reporting
>> errors, any result reported by scmi_reading_get so big as to be considered
>> a negative in 2-complement...
>>
>> ...and this will probably break even more the undocumented behaviours...
>>
>> Any feedback welcome !
> 
> Hi,
> 
> any feedback on this ? (...before I forgot again :D)

Sorry for the lag, I threw these into a build and the first thing that 
popped is the following warning on a 32-bit ARM build:

In file included from ./include/linux/bits.h:6,
                  from ./include/linux/bitops.h:6,
                  from ./include/linux/hwmon.h:15,
                  from drivers/hwmon/scmi-hwmon.c:9:
drivers/hwmon/scmi-hwmon.c: In function 'scmi_hwmon_read':
./include/vdso/bits.h:7:26: warning: left shift count >= width of type 
[-Wshift-count-overflow]
  #define BIT(nr)   (UL(1) << (nr))
                           ^~
drivers/hwmon/scmi-hwmon.c:88:14: note: in expansion of macro 'BIT'
   if (value & BIT(63)) {
               ^~~

Now, in terms of functional testing it did seems to work as intended for 
32-bit kernels not for 64-bit kernels where I got:

# sensors
scmi_sensors-virtual-0
Adapter: Virtual device
[   16.413590] hwmon hwmon0: Reported unsigned value too big.
ERROR: Can't get value of subfeature temp1_input: I/O error
avs_pvt_temp:         N/A
pmic_die_temp:    +53.4 C

whereas 32-bit would return the following:

# sensors
scmi_sensors-virtual-0
Adapter: Virtual device
avs_pvt_temp:      -6.7 C
pmic_die_temp:    +52.3 C

The firmware is version 1:

[    0.044969] arm-scmi brcm_scmi@0: SCMI Protocol v1.0 'brcm-scmi:' 
Firmware version 0x1

I will need to revisit the specification to make sure I implemented it 
correctly the first time in our version of TF-A.
-- 
Florian