Re: [PATCH] ALSA: asihpi: work around clang-17+ false positive fortify-string warning

[Takashi Iwai]

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@xxxxxxxx>
> >> 
> >> 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;