Re: dell-smm-hwmon: security problems

2016-06-08 Thread Pali Rohár 
On Wednesday 08 June 2016 19:37:43 Guenter Roeck wrote:
> On Wed, Jun 08, 2016 at 03:55:48PM +0200, Pali Rohár wrote:
> > And do you have idea what to do with problem 1)?
> 
> If you really want to do something about it, you could whiteout the
> serial number if CAP_SYS_ADMIN is not set.

Ok, that sounds reasonable. 

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.

Re: [PATCH] hwmon: (tmp401) Add support for TI TMP461

2016-06-08 Thread Guenter Roeck 
On Wed, Jun 08, 2016 at 11:56:29AM -0500, Andrew F. Davis wrote:

[ ... ]

> >>   static const u8 TMP432_TEMP_MSB_READ[4][3] = {
> >> @@ -149,6 +156,7 @@ static const struct i2c_device_id tmp401_id[] = {
> >>   { "tmp431", tmp431 },
> >>   { "tmp432", tmp432 },
> >>   { "tmp435", tmp435 },
> >> +{ "tmp461", tmp461 },
> > 
> > Please also provide code in the detect function to auto-detect the chip.
> > 
> 
> It looks like the ID reg has been removed (at least from the datasheet)
> so I'm not sure there is any good way to ID this part.
> 
Outch :-(

Well, nothing we can do about that. Even if the register was there and provides
a value, it would be undocumented and thus unreliable and unusable. Too bad.

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: dell-smm-hwmon: security problems

2016-06-08 Thread Guenter Roeck 
On Wed, Jun 08, 2016 at 03:55:48PM +0200, Pali Rohár wrote:
> On Wednesday 08 June 2016 15:24:10 Guenter Roeck wrote:
> > On 06/08/2016 02:57 AM, Pali Rohár wrote:
> > > Hello!
> > > 
> > > Mario wrote me about two I think security problems in
> > > dell-smm-hwmon driver and I would like to ask you, how to fix
> > > them.
> > > 
> > > 1) File /proc/i8k (exists only when kernel is compiled with
> > > CONFIG_I8K) exports DMI_PRODUCT_SERIAL and it can be read by
> > > ordinary user, without root permission. Normally
> > > DMI_PRODUCT_SERIAL can be read from sysfs file
> > > /sys/class/dmi/id/product_serial but only by root user.
> > > 
> > > 2) Via /proc/i8k ordinary user can set fan speed. This is because
> > > how "restricted" parameter and variable works. Setting fan speed
> > > by normal non-root user can be dangerous, e.g. malicious
> > > application under user "nobody" could take control of fans.
> > > 
> > > Do you have idea how to fix these problems? Just to note that
> > > /proc/i8k has stable kernel ABI and changing it will break all
> > > existing i8k* applications. But /proc/i8k is there only for old
> > > legacy laptops (year 2000).
> > > 
> > > There is module parameter "restricted" with default value false and
> > > description: "Allow fan control if SYS_ADMIN capability set".
> > > Current code do:
> > >
> > >   case I8K_SET_FAN:
> > >   if (restricted && !capable(CAP_SYS_ADMIN))
> > >   return -EPERM;
> > > 
> > > For me description is a bit ambiguous. What about setting
> > > "restricted" by default to true and updating description to
> > > something like this?
> > > 
> > > "Disallow fan control when SYS_ADMIN capability is not set
> > > (default: 1)"
> > 
> > Sure. I am sure that someone will complain (we learned just recently
> > that people still use the old commands, after all), but then the old
> > behavior can be restored by setting the flag to 0.
> 
> Either setting that flag to 0 or running that tool under root or with 
> capability CAP_SYS_ADMIN.
> 
> > I would not use a double negative to describe it. Why not just
> > something like "Allow fan control only if SYS_ADMIN capability set
> > (default 1)" ?
> 
> I was thinking about that description too, but there is problem with 
> meaning too...
> 
> 0 means fan control is allowed for any user
> 1 means fan control is allowed only for CAP_SYS_ADMIN
> 
> Description should be unambiguous for situation when flag is set to 0.
> 
Sorry, I don't understand how a double negation "disallow ... if not set"
would make things less ambiguous than "allow ... only if set".

> ===
> 
> And do you have idea what to do with problem 1)?
> 

If you really want to do something about it, you could whiteout the serial
number if CAP_SYS_ADMIN is not set.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hwmon: (tmp401) Add support for TI TMP461

2016-06-08 Thread Andrew F. Davis 
On 06/03/2016 11:41 PM, Guenter Roeck wrote:
> On 05/31/2016 09:27 AM, Andrew F. Davis wrote:
>> Signed-off-by: Andrew F. Davis 
>> ---
>>   Documentation/hwmon/tmp401 | 18 +--
>>   drivers/hwmon/Kconfig  |  2 +-
>>   drivers/hwmon/tmp401.c | 81
>> ++
>>   3 files changed, 92 insertions(+), 9 deletions(-)
>>
>> diff --git a/Documentation/hwmon/tmp401 b/Documentation/hwmon/tmp401
>> index 711f75e..eaa2d3b 100644
>> --- a/Documentation/hwmon/tmp401
>> +++ b/Documentation/hwmon/tmp401
>> @@ -22,6 +22,9 @@ Supported chips:
>>   Prefix: 'tmp435'
>>   Addresses scanned: I2C 0x48 - 0x4f
>>   Datasheet: http://focus.ti.com/docs/prod/folders/print/tmp435.html
>> +  * Texas Instruments TMP461
>> +Prefix: 'tmp461'
>> +Datasheet: http://www.ti.com/product/tmp461
>>
>>   Authors:
>>Hans de Goede 
>> @@ -31,8 +34,8 @@ Description
>>   ---
>>
>>   This driver implements support for Texas Instruments TMP401, TMP411,
>> -TMP431, TMP432 and TMP435 chips. These chips implement one or two remote
>> -and one local temperature sensors. Temperature is measured in degrees
>> +TMP431, TMP432, TMP435, and TMP461 chips. These chips implement one
>> or two
>> +remote and one local temperature sensors. Temperature is measured in
>> degrees
>>   Celsius. Resolution of the remote sensor is 0.0625 degree. Local
>>   sensor resolution can be set to 0.5, 0.25, 0.125 or 0.0625 degree (not
>>   supported by the driver so far, so using the default resolution of 0.5
>> @@ -55,3 +58,14 @@ some additional features.
>>
>>   TMP432 is compatible with TMP401 and TMP431. It supports two external
>>   temperature sensors.
>> +
>> +TMP461 is compatible with TMP401. It supports offset and n-factor
>> correction
>> +that is applied to the remote sensor.
>> +
>> +* Sensor offset values are temperature values
>> +
>> +  Exported via sysfs attribute tempX_offset
>> +
>> +* n-factor correction, values are in raw form as described in the
>> datasheet
>> +
> 
> If you don't mind, I would prefer if you would drop this attribute, at
> least
> for now, for a number of reasons. It should not really be controlled by
> a sysfs
> attribute, but either with devicetree data or platform data. Exposing a raw
> register value through sysfs isn't really desirable; sysfs is supposed
> to provide some kind of abstraction. It enables setting the n-factor,
> but not beta-compensation which is probably equally desirable.
> Last but not least, other chips supported by this driver, as well as
> other chips
> supported by hwmon, also support  n-factor correction and
> beta-compensation.
> If we provide this functionality we should provide it for all chips in a
> generic way. In short, this needs more discussion.
> 

Makes sense, will drop this attribute for now.

> Couple of additional additional comments below.
> 
> Thanks,
> Guenter
> 
> 
>> +  Exported via sysfs attribute tempX_nfactor
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index ff94007..c571dcf 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -1561,7 +1561,7 @@ config SENSORS_TMP401
>>   depends on I2C
>>   help
>> If you say yes here you get support for Texas Instruments TMP401,
>> -  TMP411, TMP431, TMP432 and TMP435 temperature sensor chips.
>> +  TMP411, TMP431, TMP432, TMP435, and TMP461 temperature sensor
>> chips.
>>
>> This driver can also be built as a module.  If so, the module
>> will be called tmp401.
>> diff --git a/drivers/hwmon/tmp401.c b/drivers/hwmon/tmp401.c
>> index ccf4cff..280065b 100644
>> --- a/drivers/hwmon/tmp401.c
>> +++ b/drivers/hwmon/tmp401.c
>> @@ -47,7 +47,8 @@
>>   static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4c,
>> 0x4d,
>>   0x4e, 0x4f, I2C_CLIENT_END };
>>
>> -enum chips { tmp401, tmp411, tmp431, tmp432, tmp435 };
>> +enum chips { tmp401, tmp411, tmp431, tmp432, tmp435, tmp461 };
>> +
>>
> Please no double empty lines.
> 

ACK

>>   /*
>>* The TMP401 registers, note some registers have different
>> addresses for
>> @@ -62,31 +63,37 @@ enum chips { tmp401, tmp411, tmp431, tmp432,
>> tmp435 };
>>   #define TMP401_MANUFACTURER_ID_REG0xFE
>>   #define TMP401_DEVICE_ID_REG0xFF
>>
>> -static const u8 TMP401_TEMP_MSB_READ[6][2] = {
>> +static const u8 TMP401_TEMP_MSB_READ[8][2] = {
>>   { 0x00, 0x01 },/* temp */
>>   { 0x06, 0x08 },/* low limit */
>>   { 0x05, 0x07 },/* high limit */
>>   { 0x20, 0x19 },/* therm (crit) limit */
>>   { 0x30, 0x34 },/* lowest */
>>   { 0x32, 0x36 },/* highest */
>> +{ 0, 0x11 },/* offset */
>> +{ 0, 0x23 },/* nfactor */
>>   };
>>
>> -static const u8 TMP401_TEMP_MSB_WRITE[6][2] = {
>> +static const u8 TMP401_TEMP_MSB_WRITE[8][2] = {
>>   { 0, 0 },/* temp (unused) */
>>   { 0x0C, 0x0E },/* low limit */
>>   { 0x0B,

Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver for TI INA3221

2016-06-08 Thread Laxman Dewangan 


On Wednesday 08 June 2016 08:34 PM, Andrew F. Davis wrote:

On 06/07/2016 05:30 PM, Guenter Roeck wrote:

On Fri, Jun 03, 2016 at 10:17:55AM -0500, Andrew F. Davis wrote:

On 06/03/2016 09:14 AM, Laxman Dewangan wrote:

On Friday 03 June 2016 06:59 PM, Guenter Roeck wrote:

On 06/03/2016 03:06 AM, Jonathan Cameron wrote:

On 01/06/16 13:34, Laxman Dewangan wrote:

The INA3221 is a three-channel, high-side current and bus voltage
monitor
with an I2C interface from Texas Instruments. The INA3221 monitors both
shunt voltage drops and bus supply voltages in addition to having
programmable conversion times and averaging modes for these signals.
The INA3221 offers both critical and warning alerts to detect multiple
programmable out-of-range conditions for each channel.

Add support for INA3221 SW driver via IIO ADC interface. The device is
register as iio-device and provides interface for voltage/current
and power
monitor. Also provide interface for setting oneshot/continuous mode and
critical/warning threshold for the shunt voltage drop.

Signed-off-by: Laxman Dewangan 

Hi Laxman,

As ever with any driver lying on the border of IIO and hwmon, please
include
a short justification of why you need an IIO driver and also cc the
hwmon list + maintainers. (cc'd on this reply).

I simply won't take a driver where the hwmon maintainers aren't happy.
As it stands I'm not seeing obvious reasons in the code for why this
should be an IIO device.


Me not either.

I have a hwmon driver for the same chip pending from Andrew Davis (TI)
which I am just about to accept. We had directed Andrew back in April
to write a hwmon driver for the chip, which he did.


Thanks Guenter,  I found the series

[PATCH v2 0/2] Add support for INA3221 Triple Current/Voltage Monitors
Looks fine to me. I can use the hwmon.


If you search even further back you can see I originally went with an
IIO driver as well, comparing the IIO and hwmon version for the simple
use cases I needed the hwmod version turned out much simpler, so it was
probably the right framework for now.


However, some of the stuff from my patch are not there which I will add
later once original patch applied:
- Dynamic mode changes for continuous and one-shot from sysfs.
- In one shot, when try to read voltage data, do conversion and then read.


My attempt is rather minimal, to be honest I like your stab at this
driver better in some ways, especially relating to DT putting each
channel in its own node with labels, I think this is a bit more clean
and will be more extendable if/when new multi-channel monitor chips are
made.


Not sure whether exporting the following will help or not. Can you
please confirm?
- Oversampling time i.e. average sample
- conversion time for bus voltage and shunt voltage if default is not
suited for system.



These can always be added as needed, but the DT changes are not as easy.
I would like it if this got in this cycle but if you think something
will be needed to help your improvements, that can not be added
incrementally, it would probably be best to get them into the initial DT
binding doc now.


Andrew,

with this, the hwmon driver is pretty much on hold. What do you want me to do ?
Should I wait for DT updates ?


If Laxman would like to commit to making these changes I do not problem
waiting a bit to get this right the first time.

If we don't hear back soon then I'd just take it as is and anything
needing to be fixed can be later.




I will be able to post the patch tomorrow.
I think this series can be applied and then on top of it, I will post 
the dt changes and other my changes.


As this is new driver and no one using now, it will be fine to have dt 
changes if everything complete in one cycle.


I will work from the tree on which this applied to post my patches.

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] iio: adc: ina3221: Add support for IIO ADC driver for TI INA3221

2016-06-08 Thread Andrew F. Davis 
On 06/07/2016 05:30 PM, Guenter Roeck wrote:
> On Fri, Jun 03, 2016 at 10:17:55AM -0500, Andrew F. Davis wrote:
>> On 06/03/2016 09:14 AM, Laxman Dewangan wrote:
>>>
>>> On Friday 03 June 2016 06:59 PM, Guenter Roeck wrote:
 On 06/03/2016 03:06 AM, Jonathan Cameron wrote:
> On 01/06/16 13:34, Laxman Dewangan wrote:
>> The INA3221 is a three-channel, high-side current and bus voltage
>> monitor
>> with an I2C interface from Texas Instruments. The INA3221 monitors both
>> shunt voltage drops and bus supply voltages in addition to having
>> programmable conversion times and averaging modes for these signals.
>> The INA3221 offers both critical and warning alerts to detect multiple
>> programmable out-of-range conditions for each channel.
>>
>> Add support for INA3221 SW driver via IIO ADC interface. The device is
>> register as iio-device and provides interface for voltage/current
>> and power
>> monitor. Also provide interface for setting oneshot/continuous mode and
>> critical/warning threshold for the shunt voltage drop.
>>
>> Signed-off-by: Laxman Dewangan 
> Hi Laxman,
>
> As ever with any driver lying on the border of IIO and hwmon, please
> include
> a short justification of why you need an IIO driver and also cc the
> hwmon list + maintainers. (cc'd on this reply).
>
> I simply won't take a driver where the hwmon maintainers aren't happy.
> As it stands I'm not seeing obvious reasons in the code for why this
> should be an IIO device.
>

 Me not either.

 I have a hwmon driver for the same chip pending from Andrew Davis (TI)
 which I am just about to accept. We had directed Andrew back in April
 to write a hwmon driver for the chip, which he did.

>>>
>>> Thanks Guenter,  I found the series
>>>
>>> [PATCH v2 0/2] Add support for INA3221 Triple Current/Voltage Monitors
>>> Looks fine to me. I can use the hwmon.
>>>
>>
>> If you search even further back you can see I originally went with an
>> IIO driver as well, comparing the IIO and hwmon version for the simple
>> use cases I needed the hwmod version turned out much simpler, so it was
>> probably the right framework for now.
>>
>>>
>>> However, some of the stuff from my patch are not there which I will add
>>> later once original patch applied:
>>> - Dynamic mode changes for continuous and one-shot from sysfs.
>>> - In one shot, when try to read voltage data, do conversion and then read.
>>>
>>
>> My attempt is rather minimal, to be honest I like your stab at this
>> driver better in some ways, especially relating to DT putting each
>> channel in its own node with labels, I think this is a bit more clean
>> and will be more extendable if/when new multi-channel monitor chips are
>> made.
>>
>>> Not sure whether exporting the following will help or not. Can you
>>> please confirm?
>>> - Oversampling time i.e. average sample
>>> - conversion time for bus voltage and shunt voltage if default is not
>>> suited for system.
>>>
>>>
>>
>> These can always be added as needed, but the DT changes are not as easy.
>> I would like it if this got in this cycle but if you think something
>> will be needed to help your improvements, that can not be added
>> incrementally, it would probably be best to get them into the initial DT
>> binding doc now.
>>
> Andrew,
> 
> with this, the hwmon driver is pretty much on hold. What do you want me to do 
> ?
> Should I wait for DT updates ?
> 

If Laxman would like to commit to making these changes I do not problem
waiting a bit to get this right the first time.

If we don't hear back soon then I'd just take it as is and anything
needing to be fixed can be later.

Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: dell-smm-hwmon: security problems

2016-06-08 Thread Pali Rohár 
On Wednesday 08 June 2016 15:24:10 Guenter Roeck wrote:
> On 06/08/2016 02:57 AM, Pali Rohár wrote:
> > Hello!
> > 
> > Mario wrote me about two I think security problems in
> > dell-smm-hwmon driver and I would like to ask you, how to fix
> > them.
> > 
> > 1) File /proc/i8k (exists only when kernel is compiled with
> > CONFIG_I8K) exports DMI_PRODUCT_SERIAL and it can be read by
> > ordinary user, without root permission. Normally
> > DMI_PRODUCT_SERIAL can be read from sysfs file
> > /sys/class/dmi/id/product_serial but only by root user.
> > 
> > 2) Via /proc/i8k ordinary user can set fan speed. This is because
> > how "restricted" parameter and variable works. Setting fan speed
> > by normal non-root user can be dangerous, e.g. malicious
> > application under user "nobody" could take control of fans.
> > 
> > Do you have idea how to fix these problems? Just to note that
> > /proc/i8k has stable kernel ABI and changing it will break all
> > existing i8k* applications. But /proc/i8k is there only for old
> > legacy laptops (year 2000).
> > 
> > There is module parameter "restricted" with default value false and
> > description: "Allow fan control if SYS_ADMIN capability set".
> > Current code do:
> >
> > case I8K_SET_FAN:
> > if (restricted && !capable(CAP_SYS_ADMIN))
> > return -EPERM;
> > 
> > For me description is a bit ambiguous. What about setting
> > "restricted" by default to true and updating description to
> > something like this?
> > 
> > "Disallow fan control when SYS_ADMIN capability is not set
> > (default: 1)"
> 
> Sure. I am sure that someone will complain (we learned just recently
> that people still use the old commands, after all), but then the old
> behavior can be restored by setting the flag to 0.

Either setting that flag to 0 or running that tool under root or with 
capability CAP_SYS_ADMIN.

> I would not use a double negative to describe it. Why not just
> something like "Allow fan control only if SYS_ADMIN capability set
> (default 1)" ?

I was thinking about that description too, but there is problem with 
meaning too...

0 means fan control is allowed for any user
1 means fan control is allowed only for CAP_SYS_ADMIN

Description should be unambiguous for situation when flag is set to 0.

===

And do you have idea what to do with problem 1)?

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.

Re: dell-smm-hwmon: security problems

2016-06-08 Thread Guenter Roeck 

On 06/08/2016 02:57 AM, Pali Rohár wrote:

Hello!

Mario wrote me about two I think security problems in dell-smm-hwmon
driver and I would like to ask you, how to fix them.

1) File /proc/i8k (exists only when kernel is compiled with CONFIG_I8K)
exports DMI_PRODUCT_SERIAL and it can be read by ordinary user, without
root permission. Normally DMI_PRODUCT_SERIAL can be read from sysfs file
/sys/class/dmi/id/product_serial but only by root user.

2) Via /proc/i8k ordinary user can set fan speed. This is because how
"restricted" parameter and variable works. Setting fan speed by normal
non-root user can be dangerous, e.g. malicious application under user
"nobody" could take control of fans.

Do you have idea how to fix these problems? Just to note that /proc/i8k
has stable kernel ABI and changing it will break all existing i8k*
applications. But /proc/i8k is there only for old legacy laptops (year
2000).

There is module parameter "restricted" with default value false and
description: "Allow fan control if SYS_ADMIN capability set". Current
code do:

case I8K_SET_FAN:
if (restricted && !capable(CAP_SYS_ADMIN))
return -EPERM;

For me description is a bit ambiguous. What about setting "restricted"
by default to true and updating description to something like this?

"Disallow fan control when SYS_ADMIN capability is not set (default: 1)"



Sure. I am sure that someone will complain (we learned just recently
that people still use the old commands, after all), but then the old
behavior can be restored by setting the flag to 0.

I would not use a double negative to describe it. Why not just
something like "Allow fan control only if SYS_ADMIN capability set
(default 1)" ?

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

dell-smm-hwmon: security problems

2016-06-08 Thread Pali Rohár 
Hello!

Mario wrote me about two I think security problems in dell-smm-hwmon 
driver and I would like to ask you, how to fix them.

1) File /proc/i8k (exists only when kernel is compiled with CONFIG_I8K) 
exports DMI_PRODUCT_SERIAL and it can be read by ordinary user, without 
root permission. Normally DMI_PRODUCT_SERIAL can be read from sysfs file 
/sys/class/dmi/id/product_serial but only by root user.

2) Via /proc/i8k ordinary user can set fan speed. This is because how 
"restricted" parameter and variable works. Setting fan speed by normal 
non-root user can be dangerous, e.g. malicious application under user 
"nobody" could take control of fans.

Do you have idea how to fix these problems? Just to note that /proc/i8k 
has stable kernel ABI and changing it will break all existing i8k* 
applications. But /proc/i8k is there only for old legacy laptops (year 
2000).

There is module parameter "restricted" with default value false and 
description: "Allow fan control if SYS_ADMIN capability set". Current 
code do:

case I8K_SET_FAN:
if (restricted && !capable(CAP_SYS_ADMIN))
return -EPERM;

For me description is a bit ambiguous. What about setting "restricted" 
by default to true and updating description to something like this?

"Disallow fan control when SYS_ADMIN capability is not set (default: 1)"

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.