| 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
| ||
|
Date: Sat, 12 Sep 2020 21:08:56 +0200
From: Alejandro Colomar <colomar.6.4.3@...il.com>
To: torvalds@...ux-foundation.org
Cc: akpm@...ux-foundation.org, davem@...emloft.net,
johannes.berg@...el.com, linux-kernel@...r.kernel.org,
lorenzo.bianconi83@...il.com, netdev@...r.kernel.org
Subject: Re: [GIT] Networking
On Thu, Sep 3, 2015 at 11:31 AM, Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>> [-Wsizeof-array-argument]
> Ahh. Google shows that it's an old clang warning that gcc has recently
> picked up.
> But even clang doesn't seem to have any way for a project to say
> "please warn about arrays in function argument declaration". It *is*
> very traditional idiomatic C, it's just that I personally think it's
> one of those bad traditional C things exactly because it's so
> misleading about what actually goes on. But I guess that in practice,
> the only thing that it actually *affects* is "sizeof" (and assignment
> to the variable name - something that would be invalid for a real
> array, but works on argument arrays because they are really just
> pointers).
> The "array as function argument" syntax is occasionally useful
> (particularly for the multi-dimensional array case), so I very much
> understand why it exists, I just think that in the kernel we'd be
> better off with the rule that it's against our coding practices.
> Linus
Hi Linus,
First of all, this is my first message to this mailing list, and I'm
trying to reply to a very old thread, so sorry if I don't know how/if I
should do it.
I have a different approach in my code to avoid that whole class of bugs
relating sizeof and false arrays in function argument declarations.
I do like the sintactic sugar that they provide, so I decided to ban
"sizeof(array)" completely off my code.
I have developed the following macro:
#define ARRAY_BYTES(arr) (sizeof((arr)[0]) * ARRAY_SIZE(arr))
which compiles to a simple "sizeof(arr)" by undoing the division in
"ARRAY_SIZE()", but with the added benefit that it checks that the
argument is an array (due to "ARRAY_SIZE()"), and if not, compilation
breaks which means that the array is not an array but a pointer.
My rules are:
- Size of an array (number of elements):
ARRAY_SIZE(arr)
- Signed size of an array (normally for loops where I compare against a
signed variable):
ARRAY_SSIZE(arr) defined as: ((ptrdiff_t)ARRAY_SIZE(arr))
- Size of an array in bytes (normally for buffers):
ARRAY_BYTES(arr)
No use of "sizeof" is allowed for arrays, which completely rules
out bugs of that class, because I never pass an array to "sizeof", which
is the core of the problem. I've been using those macros in my code for
more than a year, and they work really nice.
I propose to include the macro "ARRAY_BYTES()" in <linux/kernel.h> just
after "ARRAY_SIZE()" and replace every appearance of "sizeof(array)" in
Linux by "ARRAY_BYTES(array)", and modify the coding style guide to ban
"sizeof(array)" completely off the kernel.
Below are two patches: one that adds the macro to
<linux/kernel.h>, and another one that serves as an example of usage
for the macro (that one is just as an example).
I don't intend those patches to be applied directly, but instead to
be an example of what I mean. If you think the change is good, then
I'll prepare a big patch set for all of the appearances of sizeof()
that are unsafe :)
Cheers,
Alex.
------------------------------------------------------------------------
Please CC me <colomar.6.4.3@...il.com> in any response to this thread.
From b5b674d39b28e703300698fa63e4ab4be646df8f Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <colomar.6.4.3@...il.com>
Date: Sun, 5 Apr 2020 01:45:35 +0200
Subject: [PATCH 1/2] linux/kernel.h: add ARRAY_BYTES() macro
Signed-off-by: Alejandro Colomar <colomar.6.4.3@...il.com>
---
include/linux/kernel.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 9b7a8d74a9d6..dc806e2a7799 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -46,6 +46,12 @@
*/
#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
__must_be_array(arr))
+/**
+ * ARRAY_BYTES - get the number of bytes in array @arr
+ * @arr: array to be sized
+ */
+#define ARRAY_BYTES(arr) (sizeof(arr) + __must_be_array(arr))
+
#define u64_to_user_ptr(x) ( \
{ \
typecheck(u64, (x)); \
--
2.25.1
------------------------------------------------------------------------
From 3e7bcf70b708b51a7807c336c5d1b01403989d3b Mon Sep 17 00:00:00 2001
From: Alejandro Colomar <colomar.6.4.3@...il.com>
Date: Sun, 5 Apr 2020 01:48:17 +0200
Subject: [PATCH 2/2] block, bfq: Use ARRAY_BYTES() for arrays instead of
sizeof()
Signed-off-by: Alejandro Colomar <colomar.6.4.3@...il.com>
---
block/bfq-cgroup.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
index 68882b9b8f11..51ba9b9a8855 100644
--- a/block/bfq-cgroup.c
+++ b/block/bfq-cgroup.c
@@ -7,6 +7,7 @@
#include <linux/blkdev.h>
#include <linux/cgroup.h>
#include <linux/elevator.h>
+#include <linux/kernel.h>
#include <linux/ktime.h>
#include <linux/rbtree.h>
#include <linux/ioprio.h>
@@ -794,7 +795,8 @@ void bfq_bic_update_cgroup(struct bfq_io_cq *bic,
struct bio *bio)
* refcounter for bfqg, to let it disappear only after no
* bfq_queue refers to it any longer.
*/
- blkg_path(bfqg_to_blkg(bfqg), bfqg->blkg_path, sizeof(bfqg->blkg_path));
+ blkg_path(bfqg_to_blkg(bfqg), bfqg->blkg_path,
+ ARRAY_BYTES(bfqg->blkg_path));
bic->blkcg_serial_nr = serial_nr;
out:
rcu_read_unlock();
--
2.25.1
Powered by blists - more mailing lists