linux-kernel - Re: [PATCH] mfd: syscon: Set max_register_is_0 when syscon points to a single register

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: <20240828133229.67bej3utpgrmzr3p@retired>
Date: Wed, 28 Aug 2024 08:32:29 -0500
From: Nishanth Menon <nm@...com>
To: Mark Brown <broonie@...nel.org>
CC: Arnd Bergmann <arnd@...db.de>, Lee Jones <lee@...nel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <afd@...com>, <bb@...com>, <d-gole@...com>,
        Jan Dakinevich
	<jan.dakinevich@...utedevices.com>
Subject: Re: [PATCH] mfd: syscon: Set max_register_is_0 when syscon points to
 a single register

On 13:57-20240828, Mark Brown wrote:
> On Wed, Aug 28, 2024 at 07:10:08AM -0500, Nishanth Menon wrote:
> 
> > Commit 0ec74ad3c157 ("regmap: rework ->max_register handling")
> > introduced explicit handling in regmap framework for register maps
> > that is exactly 1 register wide. As a result, a syscon pointing
> > to a single register would cause regmap checks to skip checks
> > (or in the case of regmap_get_max_register, return -EINVAL) as
> > max_register_is_set will not be true.
> 
> In what sense is the behaviour changed for a map that doesn't specify a
> maximum register?
> 
> > Fixes: 0ec74ad3c157 ("regmap: rework ->max_register handling")
> 
> In what sense is this a fix?

The max_register was 0x0 was clearly a corner case. The fix done for
remap  should have cleaned up the users of max_register to maintain the
behavior. That is just my opinion.

> 
> > +	if (!syscon_config.max_register)
> > +		syscon_config.max_register_is_0 = true;
> 
> This will cause any syscon which does not explicitly specify a maximum
> register to be converted to having only one register at number 0.  That

The context of the diff is important - code above already sets
the max_register as syscon_config.max_register = resource_size(&res) -
reg_io_width;

So it already does set the max_register. syscon does'nt explictly set a
max_reg - it is derived from the resource_size.

> really does not seem like a good idea - unless you've done an audit of
> every single syscon to make sure they do explicitly specify a maximum
> register, and confirmed that this can't be specified via DT, then it's
> going to break things.

I understand the risk - but having a consistent max_register definition
is important - key here is that in regmap, max_register is valid if:
a) max_register not being 0
b) if max_register is 0, it is valid only if max_register_is_0 is set to
true.

When syscon sets the max_register, it operates correctly for num_reg > 1
however, when reg_size == 1, you don't get the checks that you
get when num_regs > 1. That is inconsistent behavior.

It might help if you can clarify why you think an inconsistent behavior
is correct for syscon?

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D