linux-kernel - Re: [PATCH] debugfs: don't access 4 bytes for a boolean

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]

Date:	Fri, 11 Sep 2015 13:18:37 +0200
From:	Rasmus Villemoes <linux@...musvillemoes.dk>
To:	Viresh Kumar <viresh.kumar@...aro.org>
Cc:	gregkh@...uxfoundation.org, linaro-kernel@...ts.linaro.org,
	Rafael Wysocki <rjw@...ysocki.net>, sboyd@...eaurora.org,
	linux-kernel@...r.kernel.org (open list)
Subject: Re: [PATCH] debugfs: don't access 4 bytes for a boolean

On Fri, Sep 11 2015, Viresh Kumar <viresh.kumar@...aro.org> wrote:

> Long back 'bool' type used to be a typecast to 'int', but that changed
> in v2.6.19. And that is a typecast to _Bool now, which (mostly) takes
> just a byte. Anyway, the bool type in kernel is used to store true/false
> or 1/0 only. So, accessing a single byte should be enough.
>
> The problem with current code is that it reads/writes 4 bytes for a
> boolean, which will read/update 3 excess bytes following the boolean
> variable. And that can lead to hard to fix bugs. It was a nightmare to
> crack this one.
>
> The debugfs code had this bug since the first time it got introduced,
> but was never got caught, strange. Maybe the bool variables (monitored
> by debugfs) were followed by an 'int' or something bigger and the pad
> bytes made sure, we never see this issue.
>
> But the OPP (Operating performance points) library have three booleans
> allocated to contiguous bytes and this bug got hit quite soon (The
> debugfs support for OPP is yet to be merged).
>
> Fix this by changing type of 'val' pointer to u8 type, so that we only
> access a single byte.

If the pointed-to type is supposed to be a bool aka _Bool, shouldn't you
cast to bool* instead of assuming sizeof(bool)==1? It's probably
non-existing, but imagine a big-endian architecture where
sizeof(bool)==4; you'd end up reading/writing the wrong byte.

> Also, there is another problem I see, which probably should be fixed as
> well. But I wanted to hear from you before trying to patch the kernel
> for this.
>
> debugfs_create_bool() declares the pointer to be of type u32 *.
> Shouldn't that be changed to u8 *? There are many users which are
> typecasting the variables to make debugfs API happy :)

Hm, yes, that's annoying. But since most people currently do pass an
u32, treating the pointer as u8* is wrong on big-endian (though of
course it doesn't matter if the value is only ever checked for being
zero/non-zero). So it would probably be better to change the
debugfs_create_bool to actually expect a bool* - there aren't _that_
many current callers, and some are obviously aware of the weirdness
(with comments such as 'must be u32 for debugfs_create_bool').

Rasmus
--
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/