linux-kernel - Re: [PATCH] regmap: fix of_regmap_get

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: <20140819052141.GA12859@ulmo>
Date:	Tue, 19 Aug 2014 07:21:45 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Stephen Warren <swarren@...dotorg.org>
Cc:	Mark Brown <broonie@...nel.org>,
	Thierry Reding <treding@...dia.com>,
	linux-kernel@...r.kernel.org, linux-next@...r.kernel.org,
	Stephen Warren <swarren@...dia.com>,
	Xiubo Li <Li.Xiubo@...escale.com>
Subject: Re: [PATCH] regmap: fix of_regmap_get_endian()

On Mon, Aug 18, 2014 at 04:14:04PM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren@...dia.com>
> 
> Commit d647c199510c ("regmap: add DT endianness binding support") has
> some issues. Specifically, if config->reg_format_endian is not explicitly
> set, it will be zero, i.e. REGMAP_ENDIAN_DEFAULT. The switch statement
> that looks up the *endian from DT for the type==REGMAP_ENDIAN_VAL case
> doesn't change *endian in the type==REGMAP_ENDIAN_REG case. However, the
> test immediately following, compares *endian against REGMAP_ENDIAN_NATIVE,
> and if not equal, returns *endian as is. This ends up returning
> REGMAP_ENDIAN_DEFAULT, which the calling code does not expect, eventually
> leading to e.g.:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 00000024
> ...
> [<c02efd64>] (regcache_cache_only) from [<c0465fa8>] (tegra30_ahub_probe+0x1b8/0x430)
> [<c0465fa8>] (tegra30_ahub_probe) from [<c02e196c>] (platform_drv_probe+0x2c/0x5c)
> [<c02e196c>] (platform_drv_probe) from [<c02e0580>] (driver_probe_device+0x10c/0x22c)
> [<c02e0580>] (driver_probe_device) from [<c02e072c>] (__driver_attach+0x8c/0x90)
> 
> This patch solves this by:
> * When looking up the endianness from DT, don't change *endian at all if
>   there is no DT property; leave it set to REGMAP_ENDIAN_DEFAULT so the
>   code falls through to other data sources in the same way as before.
>   Now, the "unspecified" case acts the same for both REGMAP_ENDIAN_REG and
>   REGMAP_ENDIAN_VAL.
> * After potentially looking up the endianness from DT, check *endian
>   against REGMAP_ENDIAN_DEFAULT instead of REGMAP_ENDIAN_NATIVE to avoid
>   returning unexpected values.
> 
> Also, clean up the code a bit:
> 
> * Make the comments briefer, and only refer to the specific action taken
>   at their location. This makes most of the comments independent of DT,
>   and easier to follow.
> * Restore the overall default of REGMAP_ENDIAN_BIG if none of the config,
>   DT, or the bus specify any endianness. Since all busses specify an
>   endianness now, this makes no difference, but I saw no justification in
>   the patch description for changing the default default.
> * s/of_regmap_get_endian/regmap_get_endian/ since the function isn't DT-
>   specific, even if the reason it was originally added was to add some
>   DT-specific features.
> 
> Reported-by: Thierry Reding <treding@...dia.com>
> Fixes: d647c199510c ("regmap: add DT endianness binding support")
> Cc: Xiubo Li <Li.Xiubo@...escale.com>
> Signed-off-by: Stephen Warren <swarren@...dia.com>
> ---
>  drivers/base/regmap/regmap.c | 58 ++++++++++++++------------------------------
>  1 file changed, 18 insertions(+), 40 deletions(-)

Thanks for fixing this, Stephen.

Tested-by: Thierry Reding <treding@...dia.com>

Content of type "application/pgp-signature" skipped