linux-kernel - Re: [RFC] pwm: core: unsigned or signed ints for pwm

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: <560D7F93.1030609@schinagl.nl>
Date:	Thu, 01 Oct 2015 20:46:43 +0200
From:	Olliver Schinagl <oliver+list@...inagl.nl>
To:	Thierry Reding <thierry.reding@...il.com>
CC:	linux-pwm@...r.kernel.org,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC] pwm: core: unsigned or signed ints for pwm_config

Hey Thierry,

On 29-09-15 09:45, Thierry Reding wrote:
> On Tue, Sep 29, 2015 at 09:19:27AM +0200, Olliver Schinagl wrote:
>> Hey Thierry, list
>>
>> I'm going over the pwm core and notice that in the pwm header, duty_ns and
>> period_ns is internally stored as an unsigned int.
>>
>> struct pwm_device {
>>      const char *label;
>>      unsigned long flags;
>>      unsigned int hwpwm;
>>      unsigned int pwm;
>>      struct pwm_chip *chip;
>>      void *chip_data;
>>
>>      unsigned int period;
>>      unsigned int duty_cycle;
>>      enum pwm_polarity polarity;
>> };
>>
>> However, pwm_config takes signed ints
>> int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);
>>
>> So digging a little deeper in the PWM core, I see that pwm_config dissallows
>> negative ints, so having them unsigned has no benefit (and technically is
>> illegal)
>>      if (!pwm  || duty_ns < 0|| period_ns= 0 || duty_ns > period_ns)
>>          return -EINVAL;
>>
>> and because (after the check) we cram the signed int into an unsigned one:
>>
>>      pwm->duty_cycle = duty_ns;
>>      pwm->period = period_ns;
>>
>> This could end up badly when using any unsigned int larger then INT_MAX and
>> thus ending up with a negative duty/period.
> I don't think this is problematic because we're rejecting negative input
> values and store the non-negative ones in an unsigned int, so we can
> never store anything that would overflow the internal representation.
>
>> I haven't checked deeper if this
>> is accounted for later, but would it be worth my time to convert all ints to
>> unsigned ints? Since negative period and duty cycles are really not possible
>> anyway?
> The reason for storing them as unsigned internally is precisely because
> they can never be negative. The reason why pwm_config() has plain ints
> is historic. It's always been on my TODO list to convert them over to a
> unsigned variant, but never high priority enough. It's also problematic
> because doing so needs to modify a public API and hence requires
> auditing all consumers and providers to make sure nothing breaks.
>
> I'm not sure if it's worth spending this effort now. Boris Brezillon
> posted patches a few weeks ago to introduce an "atomic" API and that's
> going to require updating all users anyway. The new API also uses the
> correct types, so any effort should probably go into testing and
> migrating to the new API.
Thanks for saving me from doing  alot of work herin ;)

Are Boris his patches merged in some dev tree of yours? I'm working on 
some pwm stuff too and would love to work 'with'.

Olliver
>
> Thierry

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/