linux-kernel - Re: Possible code defects: macros and precedence

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: <alpine.DEB.2.10.1609180702060.3361@hadrien>
Date:   Sun, 18 Sep 2016 07:09:36 +0200 (CEST)
From:   Julia Lawall <julia.lawall@...6.fr>
To:     Joe Perches <joe@...ches.com>
cc:     Dan Carpenter <error27@...il.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: Possible code defects: macros and precedence



On Sat, 17 Sep 2016, Joe Perches wrote:

> On Sat, 2016-09-17 at 22:24 +0200, Julia Lawall wrote:
> > diff -u -p a/lib/sha1.c b/lib/sha1.c
> []
> > @@ -49,18 +49,18 @@
> >   * the input data, the next mix it from the 512-bit array.
> >   */
> >  #define SHA_SRC(t) get_unaligned_be32((__u32 *)data + t)
> > -#define SHA_MIX(t) rol32(W(t+13) ^ W(t+8) ^ W(t+2) ^ W(t), 1)
> > +#define SHA_MIX(t) rol32(W((t)+13) ^ W((t)+8) ^ W((t)+2) ^ W(t), 1)
> >
> >  #define SHA_ROUND(t, input, fn, constant, A, B, C, D, E) do { \
> >         __u32 TEMP = input(t); setW(t, TEMP); \
> >         E += TEMP + rol32(A,5) + (fn) + (constant); \
> >         B = ror32(B, 2); } while (0)
> >
> > -#define T_0_15(t, A, B, C, D, E)  SHA_ROUND(t, SHA_SRC, (((C^D)&B)^D) , 0x5a827999, A, B, C, D, E )
> > -#define T_16_19(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, (((C^D)&B)^D) , 0x5a827999, A, B, C, D, E )
> > -#define T_20_39(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, (B^C^D) , 0x6ed9eba1, A, B, C, D, E )
> > -#define T_40_59(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, ((B&C)+(D&(B^C))) , 0x8f1bbcdc, A, B, C, D, E )
> > -#define T_60_79(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, (B^C^D) ,  0xca62c1d6, A, B, C, D, E )
> > +#define T_0_15(t, A, B, C, D, E)  SHA_ROUND(t, SHA_SRC, ((((C)^D)&B)^D) , 0x5a827999, A, B, C, D, E )
> > +#define T_16_19(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, ((((C)^D)&B)^D) , 0x5a827999, A, B, C, D, E )
> > +#define T_20_39(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, ((B)^C^D) , 0x6ed9eba1, A, B, C, D, E )
> > +#define T_40_59(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, (((B)&C)+((D)&((B)^C))) , 0x8f1bbcdc, A, B, C, D, E )
> > +#define T_60_79(t, A, B, C, D, E) SHA_ROUND(t, SHA_MIX, ((B)^C^D) ,  0xca62c1d6, A, B, C, D, E )
>
> I didn't look at much of the patch.
>
> It looks as if the coccinelle code doesn't do the
> transformation on each possible macro argument.
>
> In the transform above, only the first referenced arg
> is updated with parentheses and subsequent args are not.

No, actually it looks like only the left argument is getting transformed.
For example, T_40_59 has a term that started as ((B&C)+(D&(B^C))) and ends
up as (((B)&C)+((D)&((B)^C))).  I can fix this.

> Many or most of the uses of these various macros always
> pass a single argument to these macros so there isn't a
> real possibility of a precedence defect for those uses.
>
> Is it possible to check the macro users for arguments that
> might produce precedence defects and only report those
> possible defects?

It could be possible.  Coccinelle is not always able to associate header
files to C files, if the header files are in strange paths, but perhaps
one could also rely on the names, since some names may be unique in the
kernel.

> I also submitted a similar checkpatch addition that looks
> for non-comma operators used macro arguments in function
> definitions.
>
> The checkpatch test has the same weakness as the coccinelle
> test. It doesn't check uses, just the macro definition.

I wonder if it is really a weakness?  Does anyone care if a macro
definition has more parentheses than what is necessary for the current
usage?

julia

> Commits in -next:
>
> From 75bd22fe82d1fd698894e4a5d21e33ffdf7d4492 Mon Sep 17 00:00:00 2001
> From: Joe Perches <joe@...ches.com>
> Date: Thu, 15 Sep 2016 10:29:22 +1000
> Subject: [PATCH] checkpatch: improve MACRO_ARG_PRECEDENCE test
>
> It is possible for a multiple line macro definition to have a false positive
> report when an argument is used on a line after a continuation \.
>
> This line might have a leading '+' as the initial character that could be
> confused by checkpatch as an operator.
>
> Avoid the leading character on multiple line macro definitions.
>
> Link: http://lkml.kernel.org/r/60229d13399f9b6509db5a32e30d4c16951a60cd.1473836073.git.joe@perches.com
> Signed-off-by: Joe Perches <joe@...ches.com>
> Signed-off-by: Andrew Morton <akpm@...ux-foundation.org>
>
> From 0158682614df6006752ac932b2e65475384a87b3 Mon Sep 17 00:00:00 2001
> From: Joe Perches <joe@...ches.com>
> Date: Thu, 15 Sep 2016 10:29:22 +1000
> Subject: [PATCH] checkpatch: add --strict test for precedence challenged macro arguments
>
> Add a test for macro arguents that have a non-comma leading or trailing
> operator where the argument isn't parenthesized to avoid possible precedence
> issues.
>
> Link: http://lkml.kernel.org/r/47715508972f8d786f435e583ff881dbeee3a114.1473745855.git.joe@perches.com
> Signed-off-by: Joe Perches <joe@...ches.com>
> Cc: Andy Whitcroft <apw@...onical.com>
> Cc: Julia Lawall <julia.lawall@...6.fr>
> Cc: Dan Carpenter <dan.carpenter@...cle.com>
> Signed-off-by: Andrew Morton <akpm@...ux-foundation.org>
>
>