ALSA: asihpi: work around clang-17+ false positive fortify-string warning

  From: Arnd Bergmann <arnd@arndb.de>

One of the memory copies in this driver triggers a warning about a
possible out of range access:

In file included from /home/arnd/arm-soc/sound/pci/asihpi/hpimsgx.c:13:
/home/arnd/arm-soc/include/linux/fortify-string.h:553:4: error: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
  553 |                         __write_overflow_field(p_size_field, size);
      |                         ^

Adding a range check avoids the problem, though I don't quite see
why it warns in the first place if clang has no knowledge of the
actual range of the type, or why I never saw the warning in previous
randconfig tests.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 sound/pci/asihpi/hpimsgx.c | 3 +++
 1 file changed, 3 insertions(+)
  

On Wed, 28 Feb 2024 15:01:45 +0100,
Arnd Bergmann wrote:
> 
> From: Arnd Bergmann <arnd@arndb.de>
> 
> One of the memory copies in this driver triggers a warning about a
> possible out of range access:
> 
> In file included from /home/arnd/arm-soc/sound/pci/asihpi/hpimsgx.c:13:
> /home/arnd/arm-soc/include/linux/fortify-string.h:553:4: error: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
>   553 |                         __write_overflow_field(p_size_field, size);
>       |                         ^

Hmm, I can't see the relevance of those messages with the code you
touched.  Do you have more info?

> Adding a range check avoids the problem, though I don't quite see
> why it warns in the first place if clang has no knowledge of the
> actual range of the type, or why I never saw the warning in previous
> randconfig tests.

It's indeed puzzling.  If it's really about adapter_prepare() call,
the caller is only HPIMSGX__init(), and there is already an assignment
with that index value beforehand:
  hpi_entry_points[hr.u.s.adapter_index] = entry_point_func;

and this array is also the size of HPI_MAX_ADAPTERS.  That is, the
same check should have caught here...


thanks,

Takashi

> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  sound/pci/asihpi/hpimsgx.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/sound/pci/asihpi/hpimsgx.c b/sound/pci/asihpi/hpimsgx.c
> index d0caef299481..4f245c3956b1 100644
> --- a/sound/pci/asihpi/hpimsgx.c
> +++ b/sound/pci/asihpi/hpimsgx.c
> @@ -576,6 +576,9 @@ static u16 adapter_prepare(u16 adapter)
>  	/* Open the adapter and streams */
>  	u16 i;
>  
> +	if (adapter >= ARRAY_SIZE(rESP_HPI_ADAPTER_OPEN))
> +		return 0;
> +
>  	/* call to HPI_ADAPTER_OPEN */
>  	hpi_init_message_response(&hm, &hr, HPI_OBJ_ADAPTER,
>  		HPI_ADAPTER_OPEN);
> -- 
> 2.39.2
>
  

On Wed, Feb 28, 2024, at 15:37, Takashi Iwai wrote:
> On Wed, 28 Feb 2024 15:01:45 +0100,
> Arnd Bergmann wrote:
>> 
>> From: Arnd Bergmann <arnd@arndb.de>
>> 
>> One of the memory copies in this driver triggers a warning about a
>> possible out of range access:
>> 
>> In file included from /home/arnd/arm-soc/sound/pci/asihpi/hpimsgx.c:13:
>> /home/arnd/arm-soc/include/linux/fortify-string.h:553:4: error: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
>>   553 |                         __write_overflow_field(p_size_field, size);
>>       |                         ^
>
> Hmm, I can't see the relevance of those messages with the code you
> touched.  Do you have more info?

Not much. The warning is caused by this line:

        memcpy(&rESP_HPI_ADAPTER_OPEN[adapter], &hr,
                sizeof(rESP_HPI_ADAPTER_OPEN[0]));

rESP_HPI_ADAPTER_OPEN[] is a global array with a fixed
length of 20 elements, and 'adapter' is a 16-bit index
into that array. The warning is intended to trigger when
there is a code path that will overflow, but it normally
warns only if there is a known value range that the
variable can take, not for an unrestricted index.

My first thought was that clang warns about it here because
the 'u16 adapter' declaration limits the index to something
smaller than an 'int' or 'long', but changing the type
did not get rid of the warning.

>> Adding a range check avoids the problem, though I don't quite see
>> why it warns in the first place if clang has no knowledge of the
>> actual range of the type, or why I never saw the warning in previous
>> randconfig tests.
>
> It's indeed puzzling.  If it's really about adapter_prepare() call,
> the caller is only HPIMSGX__init(), and there is already an assignment
> with that index value beforehand:
>   hpi_entry_points[hr.u.s.adapter_index] = entry_point_func;
>
> and this array is also the size of HPI_MAX_ADAPTERS.  That is, the
> same check should have caught here...

The fortified-string warning only triggers for string.h operations
(memset, memcpy, memcmp, strn*...), not for a direct assignment.

      Arnd
  

On Wed, 28 Feb 2024 16:03:56 +0100,
Arnd Bergmann wrote:
> 
> On Wed, Feb 28, 2024, at 15:37, Takashi Iwai wrote:
> > On Wed, 28 Feb 2024 15:01:45 +0100,
> > Arnd Bergmann wrote:
> >> 
> >> From: Arnd Bergmann <arnd@arndb.de>
> >> 
> >> One of the memory copies in this driver triggers a warning about a
> >> possible out of range access:
> >> 
> >> In file included from /home/arnd/arm-soc/sound/pci/asihpi/hpimsgx.c:13:
> >> /home/arnd/arm-soc/include/linux/fortify-string.h:553:4: error: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
> >>   553 |                         __write_overflow_field(p_size_field, size);
> >>       |                         ^
> >
> > Hmm, I can't see the relevance of those messages with the code you
> > touched.  Do you have more info?
> 
> Not much. The warning is caused by this line:
> 
>         memcpy(&rESP_HPI_ADAPTER_OPEN[adapter], &hr,
>                 sizeof(rESP_HPI_ADAPTER_OPEN[0]));
> 
> rESP_HPI_ADAPTER_OPEN[] is a global array with a fixed
> length of 20 elements, and 'adapter' is a 16-bit index
> into that array. The warning is intended to trigger when
> there is a code path that will overflow, but it normally
> warns only if there is a known value range that the
> variable can take, not for an unrestricted index.
> 
> My first thought was that clang warns about it here because
> the 'u16 adapter' declaration limits the index to something
> smaller than an 'int' or 'long', but changing the type
> did not get rid of the warning.
> 
> >> Adding a range check avoids the problem, though I don't quite see
> >> why it warns in the first place if clang has no knowledge of the
> >> actual range of the type, or why I never saw the warning in previous
> >> randconfig tests.
> >
> > It's indeed puzzling.  If it's really about adapter_prepare() call,
> > the caller is only HPIMSGX__init(), and there is already an assignment
> > with that index value beforehand:
> >   hpi_entry_points[hr.u.s.adapter_index] = entry_point_func;
> >
> > and this array is also the size of HPI_MAX_ADAPTERS.  That is, the
> > same check should have caught here...
> 
> The fortified-string warning only triggers for string.h operations
> (memset, memcpy, memcmp, strn*...), not for a direct assignment.

Ah, I see.  Then from the logical POV, it's better to have a check
before that assignment; otherwise it'd overflow silently there.

Does putting the check beforehand (like the one below) fix similarly?


thanks,

Takashi

--- a/sound/pci/asihpi/hpimsgx.c
+++ b/sound/pci/asihpi/hpimsgx.c
@@ -708,7 +708,8 @@ static u16 HPIMSGX__init(struct hpi_message *phm,
 		phr->error = HPI_ERROR_PROCESSING_MESSAGE;
 		return phr->error;
 	}
-	if (hr.error == 0) {
+	if (hr.error == 0 &&
+	    hr.u.s.adapter_index < ARRAY_SIZE(rESP_HPI_ADAPTER_OPEN)) {
 		/* the adapter was created successfully
 		   save the mapping for future use */
 		hpi_entry_points[hr.u.s.adapter_index] = entry_point_func;
  

On Wed, Feb 28, 2024, at 17:24, Takashi Iwai wrote:
> On Wed, 28 Feb 2024 16:03:56 +0100,
>> On Wed, Feb 28, 2024, at 15:37, Takashi Iwai wrote:
>> > On Wed, 28 Feb 2024 15:01:45 +0100,
>> 
>> The fortified-string warning only triggers for string.h operations
>> (memset, memcpy, memcmp, strn*...), not for a direct assignment.
>
> Ah, I see.  Then from the logical POV, it's better to have a check
> before that assignment; otherwise it'd overflow silently there.
>
> Does putting the check beforehand (like the one below) fix similarly?

Indeed, it does address the issue. I'll send a v2 with that
version, since it clearly makes more sense.

       Arnd
  

On Wed, Feb 28, 2024 at 04:03:56PM +0100, Arnd Bergmann wrote:
> My first thought was that clang warns about it here because
> the 'u16 adapter' declaration limits the index to something
> smaller than an 'int' or 'long', but changing the type
> did not get rid of the warning.

Our current theory is that Clang has a bug with
__builtin_object_size/__builtin_dynamic_object_size when doing loop
unrolling (or other kinds of execution flow splitting). Some examples:
https://github.com/ClangBuiltLinux/linux/issues?q=label%3A%22loop+unroller%22+

Which is perhaps related to __bos miscalculations:
https://github.com/ClangBuiltLinux/linux/issues?q=label%3A%22%5B__bos%5D+miscalculation%22+

-Kees
  

On Wed, Feb 28, 2024 at 9:39 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Feb 28, 2024 at 04:03:56PM +0100, Arnd Bergmann wrote:
> > My first thought was that clang warns about it here because
> > the 'u16 adapter' declaration limits the index to something
> > smaller than an 'int' or 'long', but changing the type
> > did not get rid of the warning.
>
> Our current theory is that Clang has a bug with
> __builtin_object_size/__builtin_dynamic_object_size when doing loop
> unrolling (or other kinds of execution flow splitting). Some examples:
> https://github.com/ClangBuiltLinux/linux/issues?q=label%3A%22loop+unroller%22+
>
> Which is perhaps related to __bos miscalculations:
> https://github.com/ClangBuiltLinux/linux/issues?q=label%3A%22%5B__bos%5D+miscalculation%22+
>
The idea that there's a bug with the __b{d}os builtins is
controversial. The consensus among GCC and Clang compiler developers
is that returning *a* valid size, rather than the one asked for, is
okay as long as it doesn't go past the current object's max size. (I
couldn't disagree more.) There are a lot of situations where Clang
reverts to that behavior. I'm working to change that.

-bw