[PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11

[PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11

[Jan Beulich]

The upcoming release complains, not entirely unreasonably:

In file included from rijndael.c:33:
.../xen/include/crypto/rijndael.h:55:53: note: previously declared as 'const unsigned char[]'
   55 | void    rijndaelEncrypt(const unsigned int [], int, const unsigned char [],
      |                                                     ^~~~~~~~~~~~~~~~~~~~~~
rijndael.c:865:8: error: argument 4 of type 'u8[16]' {aka 'unsigned char[16]'} with mismatched bound [-Werror=array-parameter=]
  865 |     u8 ct[16])
      |     ~~~^~~~~~
In file included from rijndael.c:33:
.../xen/include/crypto/rijndael.h:56:13: note: previously declared as 'unsigned char[]'
   56 |             unsigned char []);
      |             ^~~~~~~~~~~~~~~~

While it's not really clear to me why it would complain only for arg 4,
the adjustment to make is obvious and riskfree also for arg 3: Simply
declare the correct array dimension right away. This then allows
compilers to apply checking at call sites, which seems desirable anyway.

For the moment I'm leaving untouched the disagreement between u8/u32
used in the function definition and unsigned {char,int} used in the
declaration, as making this consistent would call for touching further
functions.

Reported-by: Charles Arnold <carnold@suse.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
There are quite a few more issues with gcc11, but from my brief initial
inspection I'm suspecting (hoping) it'll rather be the compiler which
will get further changed by the time their release gets finalized. Just
one example:

.../xen/include/xen/string.h:101:27: error: '__builtin_memcmp' specified bound 4 exceeds source size 0 [-Werror=stringop-overread]
  101 | #define memcmp(s1, s2, n) __builtin_memcmp(s1, s2, n)
      |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~
mpparse.c:722:13: note: in expansion of macro 'memcmp'
  722 |         if (memcmp(mpf->mpf_signature, "_MP_", 4) == 0 &&
      |             ^~~~~~

Clearly neither the 1st nor the 2nd argument have a "source size" of 0.

--- a/xen/include/crypto/rijndael.h
+++ b/xen/include/crypto/rijndael.h
@@ -52,7 +52,7 @@
 
 int	rijndaelKeySetupEnc(unsigned int [], const unsigned char [], int);
 int	rijndaelKeySetupDec(unsigned int [], const unsigned char [], int);
-void	rijndaelEncrypt(const unsigned int [], int, const unsigned char [],
-	    unsigned char []);
+void	rijndaelEncrypt(const unsigned int [], int, const unsigned char [16],
+	    unsigned char [16]);
 
 #endif /* __RIJNDAEL_H */

Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11

[Julien Grall]

Hi Jan,

On 01/03/2021 07:57, Jan Beulich wrote:
> The upcoming release complains, not entirely unreasonably:
> 
> In file included from rijndael.c:33:
> .../xen/include/crypto/rijndael.h:55:53: note: previously declared as 'const unsigned char[]'
>     55 | void    rijndaelEncrypt(const unsigned int [], int, const unsigned char [],
>        |                                                     ^~~~~~~~~~~~~~~~~~~~~~
> rijndael.c:865:8: error: argument 4 of type 'u8[16]' {aka 'unsigned char[16]'} with mismatched bound [-Werror=array-parameter=]
>    865 |     u8 ct[16])
>        |     ~~~^~~~~~
> In file included from rijndael.c:33:
> .../xen/include/crypto/rijndael.h:56:13: note: previously declared as 'unsigned char[]'
>     56 |             unsigned char []);
>        |             ^~~~~~~~~~~~~~~~
> 
> While it's not really clear to me why it would complain only for arg 4,
> the adjustment to make is obvious and riskfree also for arg 3: Simply
> declare the correct array dimension right away. This then allows
> compilers to apply checking at call sites, which seems desirable anyway.

I am a bit confused, if GCC is not complaining for arg3, then what is 
the following error message for:

 > In file included from rijndael.c:33:
 > .../xen/include/crypto/rijndael.h:55:53: note: previously declared as 
'const unsigned char[]'
 >     55 | void    rijndaelEncrypt(const unsigned int [], int, const 
unsigned char [],
 >        | 
^~~~~~~~~~~~~~~~~~~~~~

> For the moment I'm leaving untouched the disagreement between u8/u32
> used in the function definition and unsigned {char,int} used in the
> declaration, as making this consistent would call for touching further
> functions.
> 
> Reported-by: Charles Arnold <carnold@suse.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> There are quite a few more issues with gcc11, but from my brief initial
> inspection I'm suspecting (hoping) it'll rather be the compiler which
> will get further changed by the time their release gets finalized. Just
> one example:
> 
> .../xen/include/xen/string.h:101:27: error: '__builtin_memcmp' specified bound 4 exceeds source size 0 [-Werror=stringop-overread]
>    101 | #define memcmp(s1, s2, n) __builtin_memcmp(s1, s2, n)
>        |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> mpparse.c:722:13: note: in expansion of macro 'memcmp'
>    722 |         if (memcmp(mpf->mpf_signature, "_MP_", 4) == 0 &&
>        |             ^~~~~~
> 
> Clearly neither the 1st nor the 2nd argument have a "source size" of 0.

It looks like there is a report on the redhat bug tracker for it [1]. Do 
you know if there is a bug report on the GCC tracker as well?

> 
> --- a/xen/include/crypto/rijndael.h
> +++ b/xen/include/crypto/rijndael.h
> @@ -52,7 +52,7 @@
>   
>   int	rijndaelKeySetupEnc(unsigned int [], const unsigned char [], int);
>   int	rijndaelKeySetupDec(unsigned int [], const unsigned char [], int);
> -void	rijndaelEncrypt(const unsigned int [], int, const unsigned char [],
> -	    unsigned char []);
> +void	rijndaelEncrypt(const unsigned int [], int, const unsigned char [16],
> +	    unsigned char [16]);
>   
>   #endif /* __RIJNDAEL_H */
> 

Cheers,

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1892100

-- 
Julien Grall

Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11

[Jan Beulich]

On 03.03.2021 20:09, Julien Grall wrote:
> On 01/03/2021 07:57, Jan Beulich wrote:
>> The upcoming release complains, not entirely unreasonably:
>>
>> In file included from rijndael.c:33:
>> .../xen/include/crypto/rijndael.h:55:53: note: previously declared as 'const unsigned char[]'
>>     55 | void    rijndaelEncrypt(const unsigned int [], int, const unsigned char [],
>>        |                                                     ^~~~~~~~~~~~~~~~~~~~~~
>> rijndael.c:865:8: error: argument 4 of type 'u8[16]' {aka 'unsigned char[16]'} with mismatched bound [-Werror=array-parameter=]
>>    865 |     u8 ct[16])
>>        |     ~~~^~~~~~
>> In file included from rijndael.c:33:
>> .../xen/include/crypto/rijndael.h:56:13: note: previously declared as 'unsigned char[]'
>>     56 |             unsigned char []);
>>        |             ^~~~~~~~~~~~~~~~
>>
>> While it's not really clear to me why it would complain only for arg 4,
>> the adjustment to make is obvious and riskfree also for arg 3: Simply
>> declare the correct array dimension right away. This then allows
>> compilers to apply checking at call sites, which seems desirable anyway.
> 
> I am a bit confused, if GCC is not complaining for arg3, then what is 
> the following error message for:
> 
>  > In file included from rijndael.c:33:
>  > .../xen/include/crypto/rijndael.h:55:53: note: previously declared as 
> 'const unsigned char[]'
>  >     55 | void    rijndaelEncrypt(const unsigned int [], int, const 
> unsigned char [],
>  >        | 
> ^~~~~~~~~~~~~~~~~~~~~~

Hmm, good point. I didn't observe this myself, and simply copied the
part of the error message that I was handed. I suppose there was an
"error: argument 3 of type ..." there then as well. Charles - any
chance you could confirm this, and perhaps even quote the full set
of error messages in our internal patch?

I'll adjust the wording of the description in any event.

>> There are quite a few more issues with gcc11, but from my brief initial
>> inspection I'm suspecting (hoping) it'll rather be the compiler which
>> will get further changed by the time their release gets finalized. Just
>> one example:
>>
>> .../xen/include/xen/string.h:101:27: error: '__builtin_memcmp' specified bound 4 exceeds source size 0 [-Werror=stringop-overread]
>>    101 | #define memcmp(s1, s2, n) __builtin_memcmp(s1, s2, n)
>>        |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>> mpparse.c:722:13: note: in expansion of macro 'memcmp'
>>    722 |         if (memcmp(mpf->mpf_signature, "_MP_", 4) == 0 &&
>>        |             ^~~~~~
>>
>> Clearly neither the 1st nor the 2nd argument have a "source size" of 0.
> 
> It looks like there is a report on the redhat bug tracker for it [1]. Do 
> you know if there is a bug report on the GCC tracker as well?

I have no idea, to be honest.

Jan

Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11

[Ian Jackson]

Jan Beulich writes ("Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11"):
> On 03.03.2021 20:09, Julien Grall wrote:
> > On 01/03/2021 07:57, Jan Beulich wrote:
> >> The upcoming release complains, not entirely unreasonably:
> >>
> >> In file included from rijndael.c:33:
> >> .../xen/include/crypto/rijndael.h:55:53: note: previously declared as 'const unsigned char[]'
> >>     55 | void    rijndaelEncrypt(const unsigned int [], int, const unsigned char [],
> >>        |                                                     ^~~~~~~~~~~~~~~~~~~~~~
> >> rijndael.c:865:8: error: argument 4 of type 'u8[16]' {aka 'unsigned char[16]'} with mismatched bound [-Werror=array-parameter=]
> >>    865 |     u8 ct[16])

I think this is an erroneous compiler warning.

It has been idiomatic in some codebases for a long time to write
    const unsigned char[]
as a formal parameter for an array (of whatever size).

> >> .../xen/include/xen/string.h:101:27: error: '__builtin_memcmp' specified bound 4 exceeds source size 0 [-Werror=stringop-overread]
> >>    101 | #define memcmp(s1, s2, n) __builtin_memcmp(s1, s2, n)
> >>        |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> mpparse.c:722:13: note: in expansion of macro 'memcmp'
> >>    722 |         if (memcmp(mpf->mpf_signature, "_MP_", 4) == 0 &&
> >>        |             ^~~~~~
> >>
> >> Clearly neither the 1st nor the 2nd argument have a "source size" of 0.
> > 
> > It looks like there is a report on the redhat bug tracker for it [1]. Do 
> > you know if there is a bug report on the GCC tracker as well?
> 
> I have no idea, to be honest.

This erroneous message makes me think that there is simply a bug in
this version of GCC, where formal parameters declared as
    const unsigned char[]
are treated as
    const unsigned char[0]
rather than as
    const unsigned char*

Ian.

Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11

[Julien Grall]

On 04/03/2021 08:06, Jan Beulich wrote:
> On 03.03.2021 20:09, Julien Grall wrote:
>> On 01/03/2021 07:57, Jan Beulich wrote:
>>> The upcoming release complains, not entirely unreasonably:
>>>
>>> In file included from rijndael.c:33:
>>> .../xen/include/crypto/rijndael.h:55:53: note: previously declared as 'const unsigned char[]'
>>>      55 | void    rijndaelEncrypt(const unsigned int [], int, const unsigned char [],
>>>         |                                                     ^~~~~~~~~~~~~~~~~~~~~~
>>> rijndael.c:865:8: error: argument 4 of type 'u8[16]' {aka 'unsigned char[16]'} with mismatched bound [-Werror=array-parameter=]
>>>     865 |     u8 ct[16])
>>>         |     ~~~^~~~~~
>>> In file included from rijndael.c:33:
>>> .../xen/include/crypto/rijndael.h:56:13: note: previously declared as 'unsigned char[]'
>>>      56 |             unsigned char []);
>>>         |             ^~~~~~~~~~~~~~~~
>>>
>>> While it's not really clear to me why it would complain only for arg 4,
>>> the adjustment to make is obvious and riskfree also for arg 3: Simply
>>> declare the correct array dimension right away. This then allows
>>> compilers to apply checking at call sites, which seems desirable anyway.
>>
>> I am a bit confused, if GCC is not complaining for arg3, then what is
>> the following error message for:
>>
>>   > In file included from rijndael.c:33:
>>   > .../xen/include/crypto/rijndael.h:55:53: note: previously declared as
>> 'const unsigned char[]'
>>   >     55 | void    rijndaelEncrypt(const unsigned int [], int, const
>> unsigned char [],
>>   >        |
>> ^~~~~~~~~~~~~~~~~~~~~~
> 
> Hmm, good point. I didn't observe this myself, and simply copied the
> part of the error message that I was handed. I suppose there was an
> "error: argument 3 of type ..." there then as well. Charles - any
> chance you could confirm this, and perhaps even quote the full set
> of error messages in our internal patch?
> 
> I'll adjust the wording of the description in any event.

With the description adjusted:

Reviewed-by: Julien Grall <jgrall@amazon.com>

> 
>>> There are quite a few more issues with gcc11, but from my brief initial
>>> inspection I'm suspecting (hoping) it'll rather be the compiler which
>>> will get further changed by the time their release gets finalized. Just
>>> one example:
>>>
>>> .../xen/include/xen/string.h:101:27: error: '__builtin_memcmp' specified bound 4 exceeds source size 0 [-Werror=stringop-overread]
>>>     101 | #define memcmp(s1, s2, n) __builtin_memcmp(s1, s2, n)
>>>         |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> mpparse.c:722:13: note: in expansion of macro 'memcmp'
>>>     722 |         if (memcmp(mpf->mpf_signature, "_MP_", 4) == 0 &&
>>>         |             ^~~~~~
>>>
>>> Clearly neither the 1st nor the 2nd argument have a "source size" of 0.
>>
>> It looks like there is a report on the redhat bug tracker for it [1]. Do
>> you know if there is a bug report on the GCC tracker as well?
> 
> I have no idea, to be honest.

I had a look and couldn't find any. It might be worth to fill one in 
case they are not aware.

Cheers,

-- 
Julien Grall

Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11

[Julien Grall]

On 04/03/2021 11:21, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11"):
>> On 03.03.2021 20:09, Julien Grall wrote:
>>> On 01/03/2021 07:57, Jan Beulich wrote:
>>>> The upcoming release complains, not entirely unreasonably:
>>>>
>>>> In file included from rijndael.c:33:
>>>> .../xen/include/crypto/rijndael.h:55:53: note: previously declared as 'const unsigned char[]'
>>>>      55 | void    rijndaelEncrypt(const unsigned int [], int, const unsigned char [],
>>>>         |                                                     ^~~~~~~~~~~~~~~~~~~~~~
>>>> rijndael.c:865:8: error: argument 4 of type 'u8[16]' {aka 'unsigned char[16]'} with mismatched bound [-Werror=array-parameter=]
>>>>     865 |     u8 ct[16])
> 
> I think this is an erroneous compiler warning.
> 
> It has been idiomatic in some codebases for a long time to write
>      const unsigned char[]
> as a formal parameter for an array (of whatever size).
AFAICT, this is not what GCC is trying to warn about. It is complaining 
that the prototype and the declaration doesn't use the same signature.

Indeed, the former is using [] while the declaration is using [16].

So compiler is working as intended when -Warray-parameter is selected 
(see [1]).

[1] https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
-- 
Julien Grall

Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11

[Ian Jackson]

Julien Grall writes ("Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11"):
> On 04/03/2021 11:21, Ian Jackson wrote:
> > Jan Beulich writes ("Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11"):
...
> > It has been idiomatic in some codebases for a long time to write
> >      const unsigned char[]
> > as a formal parameter for an array (of whatever size).
>
> AFAICT, this is not what GCC is trying to warn about. It is complaining 
> that the prototype and the declaration doesn't use the same signature.

Oh!  I would have to check whether that is legal (I would guess
probably it is UB because the C authors hate us all) but I agree that
the warning is justified and the code should be changed.

Sorry for the misunderstanding.

Ian.

Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11

[Jan Beulich]

On 04.03.2021 12:21, Ian Jackson wrote:
> Jan Beulich writes ("Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11"):
>> On 03.03.2021 20:09, Julien Grall wrote:
>>> On 01/03/2021 07:57, Jan Beulich wrote:
>>>> The upcoming release complains, not entirely unreasonably:
>>>>
>>>> In file included from rijndael.c:33:
>>>> .../xen/include/crypto/rijndael.h:55:53: note: previously declared as 'const unsigned char[]'
>>>>     55 | void    rijndaelEncrypt(const unsigned int [], int, const unsigned char [],
>>>>        |                                                     ^~~~~~~~~~~~~~~~~~~~~~
>>>> rijndael.c:865:8: error: argument 4 of type 'u8[16]' {aka 'unsigned char[16]'} with mismatched bound [-Werror=array-parameter=]
>>>>    865 |     u8 ct[16])
> 
> I think this is an erroneous compiler warning.
> 
> It has been idiomatic in some codebases for a long time to write
>     const unsigned char[]
> as a formal parameter for an array (of whatever size).
> 
>>>> .../xen/include/xen/string.h:101:27: error: '__builtin_memcmp' specified bound 4 exceeds source size 0 [-Werror=stringop-overread]
>>>>    101 | #define memcmp(s1, s2, n) __builtin_memcmp(s1, s2, n)
>>>>        |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> mpparse.c:722:13: note: in expansion of macro 'memcmp'
>>>>    722 |         if (memcmp(mpf->mpf_signature, "_MP_", 4) == 0 &&
>>>>        |             ^~~~~~
>>>>
>>>> Clearly neither the 1st nor the 2nd argument have a "source size" of 0.
>>>
>>> It looks like there is a report on the redhat bug tracker for it [1]. Do 
>>> you know if there is a bug report on the GCC tracker as well?
>>
>> I have no idea, to be honest.
> 
> This erroneous message makes me think that there is simply a bug in
> this version of GCC, where formal parameters declared as
>     const unsigned char[]
> are treated as
>     const unsigned char[0]
> rather than as
>     const unsigned char*

I don't think so, no. In addition to what Julien has said, I think
the intention here is that the compiler can check that both consumer
and producers of arguments obey to the strictest available bounds of
such an array, no matter that at the syntactic level char[N] and
char[] both convert to char* when used in prototypes.

Jan

Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11

[Jan Beulich]

On 04.03.2021 13:41, Ian Jackson wrote:
> Julien Grall writes ("Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11"):
>> On 04/03/2021 11:21, Ian Jackson wrote:
>>> Jan Beulich writes ("Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11"):
> ...
>>> It has been idiomatic in some codebases for a long time to write
>>>      const unsigned char[]
>>> as a formal parameter for an array (of whatever size).
>>
>> AFAICT, this is not what GCC is trying to warn about. It is complaining 
>> that the prototype and the declaration doesn't use the same signature.
> 
> Oh!  I would have to check whether that is legal (I would guess
> probably it is UB because the C authors hate us all) but I agree that
> the warning is justified and the code should be changed.

May I translate this into a release ack then?

Jan

Re: [PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11

[Ian Jackson]

Jan Beulich writes ("[PATCH][4.15] crypto: adjust rijndaelEncrypt() prototype for gcc11"):
> Clearly neither the 1st nor the 2nd argument have a "source size" of 0.
> 
> --- a/xen/include/crypto/rijndael.h
> +++ b/xen/include/crypto/rijndael.h
> @@ -52,7 +52,7 @@
>  
>  int	rijndaelKeySetupEnc(unsigned int [], const unsigned char [], int);
>  int	rijndaelKeySetupDec(unsigned int [], const unsigned char [], int);
> -void	rijndaelEncrypt(const unsigned int [], int, const unsigned char [],
> -	    unsigned char []);
> +void	rijndaelEncrypt(const unsigned int [], int, const unsigned char [16],
> +	    unsigned char [16]);
>  
>  #endif /* __RIJNDAEL_H */

Release-Acked-by: Ian Jackson <iwj@xenproject.org>