| Message ID | 20240228140152.1824901-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: asihpi: work around clang-17+ false positive fortify-string warning | expand |
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
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);