* [PATCH] pnp: acpi: fix fortify warning
@ 2023-11-27 14:58 Dmitry Antipov
2023-11-27 19:01 ` Rafael J. Wysocki
0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Antipov @ 2023-11-27 14:58 UTC (permalink / raw)
To: Rafael J . Wysocki; +Cc: Len Brown, Kees Cook, linux-acpi, Dmitry Antipov
When compiling with gcc version 14.0.0 20231126 (experimental)
and CONFIG_FORTIFY_SOURCE=y, I've noticed the following:
In file included from ./include/linux/string.h:295,
from ./include/linux/bitmap.h:12,
from ./include/linux/cpumask.h:12,
from ./arch/x86/include/asm/paravirt.h:17,
from ./arch/x86/include/asm/cpuid.h:62,
from ./arch/x86/include/asm/processor.h:19,
from ./arch/x86/include/asm/cpufeature.h:5,
from ./arch/x86/include/asm/thread_info.h:53,
from ./include/linux/thread_info.h:60,
from ./arch/x86/include/asm/preempt.h:9,
from ./include/linux/preempt.h:79,
from ./include/linux/spinlock.h:56,
from ./include/linux/mmzone.h:8,
from ./include/linux/gfp.h:7,
from ./include/linux/slab.h:16,
from ./include/linux/resource_ext.h:11,
from ./include/linux/acpi.h:13,
from drivers/pnp/pnpacpi/rsparser.c:11:
In function 'fortify_memcpy_chk',
inlined from 'pnpacpi_parse_allocated_vendor' at drivers/pnp/pnpacpi/rsparser.c:158:3,
inlined from 'pnpacpi_allocated_resource' at drivers/pnp/pnpacpi/rsparser.c:249:3:
./include/linux/fortify-string.h:588:25: warning: call to '__read_overflow2_field'
declared with attribute warning: detected read beyond size of field (2nd parameter);
maybe use struct_group()? [-Wattribute-warning]
588 | __read_overflow2_field(q_size_field, size);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
According to the comments in include/linux/fortify-string.h, 'memcpy()',
'memmove()' and 'memset()' must not be used beyond individual struct
members to ensure that the compiler can enforce protection against
buffer overflows, and, IIUC, this also applies to partial copies from
the particular member ('vendor->byte_data' in this case). So it should
be better (and safer) to do both copies at once (and 'byte_data' of
'struct acpi_resource_vendor_typed' seems to be a good candidate for
'__counted_by(byte_length)' as well).
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
drivers/pnp/pnpacpi/rsparser.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/pnp/pnpacpi/rsparser.c b/drivers/pnp/pnpacpi/rsparser.c
index 4f05f610391b..af51f893525e 100644
--- a/drivers/pnp/pnpacpi/rsparser.c
+++ b/drivers/pnp/pnpacpi/rsparser.c
@@ -152,12 +152,12 @@ static void pnpacpi_parse_allocated_vendor(struct pnp_dev *dev,
struct acpi_resource_vendor_typed *vendor)
{
if (vendor_resource_matches(dev, vendor, &hp_ccsr_uuid, 16)) {
- u64 start, length;
+ struct { u64 start, length; } range;
- memcpy(&start, vendor->byte_data, sizeof(start));
- memcpy(&length, vendor->byte_data + 8, sizeof(length));
+ memcpy(&range, vendor->byte_data, 16);
- pnp_add_mem_resource(dev, start, start + length - 1, 0);
+ pnp_add_mem_resource(dev, range.start, range.start +
+ range.length - 1, 0);
}
}
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] pnp: acpi: fix fortify warning
2023-11-27 14:58 [PATCH] pnp: acpi: fix fortify warning Dmitry Antipov
@ 2023-11-27 19:01 ` Rafael J. Wysocki
2023-11-28 2:52 ` [PATCH] [v2] " Dmitry Antipov
0 siblings, 1 reply; 5+ messages in thread
From: Rafael J. Wysocki @ 2023-11-27 19:01 UTC (permalink / raw)
To: Dmitry Antipov; +Cc: Rafael J . Wysocki, Len Brown, Kees Cook, linux-acpi
On Mon, Nov 27, 2023 at 4:00 PM Dmitry Antipov <dmantipov@yandex.ru> wrote:
>
> When compiling with gcc version 14.0.0 20231126 (experimental)
> and CONFIG_FORTIFY_SOURCE=y, I've noticed the following:
>
> In file included from ./include/linux/string.h:295,
> from ./include/linux/bitmap.h:12,
> from ./include/linux/cpumask.h:12,
> from ./arch/x86/include/asm/paravirt.h:17,
> from ./arch/x86/include/asm/cpuid.h:62,
> from ./arch/x86/include/asm/processor.h:19,
> from ./arch/x86/include/asm/cpufeature.h:5,
> from ./arch/x86/include/asm/thread_info.h:53,
> from ./include/linux/thread_info.h:60,
> from ./arch/x86/include/asm/preempt.h:9,
> from ./include/linux/preempt.h:79,
> from ./include/linux/spinlock.h:56,
> from ./include/linux/mmzone.h:8,
> from ./include/linux/gfp.h:7,
> from ./include/linux/slab.h:16,
> from ./include/linux/resource_ext.h:11,
> from ./include/linux/acpi.h:13,
> from drivers/pnp/pnpacpi/rsparser.c:11:
> In function 'fortify_memcpy_chk',
> inlined from 'pnpacpi_parse_allocated_vendor' at drivers/pnp/pnpacpi/rsparser.c:158:3,
> inlined from 'pnpacpi_allocated_resource' at drivers/pnp/pnpacpi/rsparser.c:249:3:
> ./include/linux/fortify-string.h:588:25: warning: call to '__read_overflow2_field'
> declared with attribute warning: detected read beyond size of field (2nd parameter);
> maybe use struct_group()? [-Wattribute-warning]
> 588 | __read_overflow2_field(q_size_field, size);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> According to the comments in include/linux/fortify-string.h, 'memcpy()',
> 'memmove()' and 'memset()' must not be used beyond individual struct
> members to ensure that the compiler can enforce protection against
> buffer overflows, and, IIUC, this also applies to partial copies from
> the particular member ('vendor->byte_data' in this case). So it should
> be better (and safer) to do both copies at once (and 'byte_data' of
> 'struct acpi_resource_vendor_typed' seems to be a good candidate for
> '__counted_by(byte_length)' as well).
>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> drivers/pnp/pnpacpi/rsparser.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pnp/pnpacpi/rsparser.c b/drivers/pnp/pnpacpi/rsparser.c
> index 4f05f610391b..af51f893525e 100644
> --- a/drivers/pnp/pnpacpi/rsparser.c
> +++ b/drivers/pnp/pnpacpi/rsparser.c
> @@ -152,12 +152,12 @@ static void pnpacpi_parse_allocated_vendor(struct pnp_dev *dev,
> struct acpi_resource_vendor_typed *vendor)
> {
> if (vendor_resource_matches(dev, vendor, &hp_ccsr_uuid, 16)) {
> - u64 start, length;
> + struct { u64 start, length; } range;
>
> - memcpy(&start, vendor->byte_data, sizeof(start));
> - memcpy(&length, vendor->byte_data + 8, sizeof(length));
> + memcpy(&range, vendor->byte_data, 16);
sizeof(range) instead of 16 here?
>
> - pnp_add_mem_resource(dev, start, start + length - 1, 0);
> + pnp_add_mem_resource(dev, range.start, range.start +
> + range.length - 1, 0);
> }
> }
>
> --
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] [v2] pnp: acpi: fix fortify warning
2023-11-27 19:01 ` Rafael J. Wysocki
@ 2023-11-28 2:52 ` Dmitry Antipov
2023-11-30 23:29 ` Kees Cook
0 siblings, 1 reply; 5+ messages in thread
From: Dmitry Antipov @ 2023-11-28 2:52 UTC (permalink / raw)
To: Rafael J . Wysocki; +Cc: Len Brown, Kees Cook, linux-acpi, Dmitry Antipov
When compiling with gcc version 14.0.0 20231126 (experimental)
and CONFIG_FORTIFY_SOURCE=y, I've noticed the following:
In file included from ./include/linux/string.h:295,
from ./include/linux/bitmap.h:12,
from ./include/linux/cpumask.h:12,
from ./arch/x86/include/asm/paravirt.h:17,
from ./arch/x86/include/asm/cpuid.h:62,
from ./arch/x86/include/asm/processor.h:19,
from ./arch/x86/include/asm/cpufeature.h:5,
from ./arch/x86/include/asm/thread_info.h:53,
from ./include/linux/thread_info.h:60,
from ./arch/x86/include/asm/preempt.h:9,
from ./include/linux/preempt.h:79,
from ./include/linux/spinlock.h:56,
from ./include/linux/mmzone.h:8,
from ./include/linux/gfp.h:7,
from ./include/linux/slab.h:16,
from ./include/linux/resource_ext.h:11,
from ./include/linux/acpi.h:13,
from drivers/pnp/pnpacpi/rsparser.c:11:
In function 'fortify_memcpy_chk',
inlined from 'pnpacpi_parse_allocated_vendor' at drivers/pnp/pnpacpi/rsparser.c:158:3,
inlined from 'pnpacpi_allocated_resource' at drivers/pnp/pnpacpi/rsparser.c:249:3:
./include/linux/fortify-string.h:588:25: warning: call to '__read_overflow2_field'
declared with attribute warning: detected read beyond size of field (2nd parameter);
maybe use struct_group()? [-Wattribute-warning]
588 | __read_overflow2_field(q_size_field, size);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
According to the comments in include/linux/fortify-string.h, 'memcpy()',
'memmove()' and 'memset()' must not be used beyond individual struct
members to ensure that the compiler can enforce protection against
buffer overflows, and, IIUC, this also applies to partial copies from
the particular member ('vendor->byte_data' in this case). So it should
be better (and safer) to do both copies at once (and 'byte_data' of
'struct acpi_resource_vendor_typed' seems to be a good candidate for
'__counted_by(byte_length)' as well).
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v2: prefer sizeof(range) over hardcoded constant (Rafael J. Wysocki)
---
drivers/pnp/pnpacpi/rsparser.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/pnp/pnpacpi/rsparser.c b/drivers/pnp/pnpacpi/rsparser.c
index 4f05f610391b..c02ce0834c2c 100644
--- a/drivers/pnp/pnpacpi/rsparser.c
+++ b/drivers/pnp/pnpacpi/rsparser.c
@@ -151,13 +151,13 @@ static int vendor_resource_matches(struct pnp_dev *dev,
static void pnpacpi_parse_allocated_vendor(struct pnp_dev *dev,
struct acpi_resource_vendor_typed *vendor)
{
- if (vendor_resource_matches(dev, vendor, &hp_ccsr_uuid, 16)) {
- u64 start, length;
+ struct { u64 start, length; } range;
- memcpy(&start, vendor->byte_data, sizeof(start));
- memcpy(&length, vendor->byte_data + 8, sizeof(length));
-
- pnp_add_mem_resource(dev, start, start + length - 1, 0);
+ if (vendor_resource_matches(dev, vendor, &hp_ccsr_uuid,
+ sizeof(range))) {
+ memcpy(&range, vendor->byte_data, sizeof(range));
+ pnp_add_mem_resource(dev, range.start, range.start +
+ range.length - 1, 0);
}
}
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] [v2] pnp: acpi: fix fortify warning
2023-11-28 2:52 ` [PATCH] [v2] " Dmitry Antipov
@ 2023-11-30 23:29 ` Kees Cook
2023-12-06 20:03 ` Rafael J. Wysocki
0 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2023-11-30 23:29 UTC (permalink / raw)
To: Dmitry Antipov; +Cc: Rafael J . Wysocki, Len Brown, linux-acpi
On Tue, Nov 28, 2023 at 05:52:10AM +0300, Dmitry Antipov wrote:
> When compiling with gcc version 14.0.0 20231126 (experimental)
> and CONFIG_FORTIFY_SOURCE=y, I've noticed the following:
>
> In file included from ./include/linux/string.h:295,
> from ./include/linux/bitmap.h:12,
> from ./include/linux/cpumask.h:12,
> from ./arch/x86/include/asm/paravirt.h:17,
> from ./arch/x86/include/asm/cpuid.h:62,
> from ./arch/x86/include/asm/processor.h:19,
> from ./arch/x86/include/asm/cpufeature.h:5,
> from ./arch/x86/include/asm/thread_info.h:53,
> from ./include/linux/thread_info.h:60,
> from ./arch/x86/include/asm/preempt.h:9,
> from ./include/linux/preempt.h:79,
> from ./include/linux/spinlock.h:56,
> from ./include/linux/mmzone.h:8,
> from ./include/linux/gfp.h:7,
> from ./include/linux/slab.h:16,
> from ./include/linux/resource_ext.h:11,
> from ./include/linux/acpi.h:13,
> from drivers/pnp/pnpacpi/rsparser.c:11:
> In function 'fortify_memcpy_chk',
> inlined from 'pnpacpi_parse_allocated_vendor' at drivers/pnp/pnpacpi/rsparser.c:158:3,
> inlined from 'pnpacpi_allocated_resource' at drivers/pnp/pnpacpi/rsparser.c:249:3:
> ./include/linux/fortify-string.h:588:25: warning: call to '__read_overflow2_field'
> declared with attribute warning: detected read beyond size of field (2nd parameter);
> maybe use struct_group()? [-Wattribute-warning]
> 588 | __read_overflow2_field(q_size_field, size);
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> According to the comments in include/linux/fortify-string.h, 'memcpy()',
> 'memmove()' and 'memset()' must not be used beyond individual struct
> members to ensure that the compiler can enforce protection against
> buffer overflows, and, IIUC, this also applies to partial copies from
> the particular member ('vendor->byte_data' in this case). So it should
> be better (and safer) to do both copies at once (and 'byte_data' of
> 'struct acpi_resource_vendor_typed' seems to be a good candidate for
> '__counted_by(byte_length)' as well).
>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> v2: prefer sizeof(range) over hardcoded constant (Rafael J. Wysocki)
Yeah, this looks good to me. Thanks for the fix!
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> drivers/pnp/pnpacpi/rsparser.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pnp/pnpacpi/rsparser.c b/drivers/pnp/pnpacpi/rsparser.c
> index 4f05f610391b..c02ce0834c2c 100644
> --- a/drivers/pnp/pnpacpi/rsparser.c
> +++ b/drivers/pnp/pnpacpi/rsparser.c
> @@ -151,13 +151,13 @@ static int vendor_resource_matches(struct pnp_dev *dev,
> static void pnpacpi_parse_allocated_vendor(struct pnp_dev *dev,
> struct acpi_resource_vendor_typed *vendor)
> {
> - if (vendor_resource_matches(dev, vendor, &hp_ccsr_uuid, 16)) {
> - u64 start, length;
> + struct { u64 start, length; } range;
>
> - memcpy(&start, vendor->byte_data, sizeof(start));
> - memcpy(&length, vendor->byte_data + 8, sizeof(length));
> -
> - pnp_add_mem_resource(dev, start, start + length - 1, 0);
> + if (vendor_resource_matches(dev, vendor, &hp_ccsr_uuid,
> + sizeof(range))) {
> + memcpy(&range, vendor->byte_data, sizeof(range));
> + pnp_add_mem_resource(dev, range.start, range.start +
> + range.length - 1, 0);
> }
> }
>
> --
> 2.43.0
>
--
Kees Cook
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] [v2] pnp: acpi: fix fortify warning
2023-11-30 23:29 ` Kees Cook
@ 2023-12-06 20:03 ` Rafael J. Wysocki
0 siblings, 0 replies; 5+ messages in thread
From: Rafael J. Wysocki @ 2023-12-06 20:03 UTC (permalink / raw)
To: Kees Cook, Dmitry Antipov; +Cc: Rafael J . Wysocki, Len Brown, linux-acpi
On Fri, Dec 1, 2023 at 12:29 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Nov 28, 2023 at 05:52:10AM +0300, Dmitry Antipov wrote:
> > When compiling with gcc version 14.0.0 20231126 (experimental)
> > and CONFIG_FORTIFY_SOURCE=y, I've noticed the following:
> >
> > In file included from ./include/linux/string.h:295,
> > from ./include/linux/bitmap.h:12,
> > from ./include/linux/cpumask.h:12,
> > from ./arch/x86/include/asm/paravirt.h:17,
> > from ./arch/x86/include/asm/cpuid.h:62,
> > from ./arch/x86/include/asm/processor.h:19,
> > from ./arch/x86/include/asm/cpufeature.h:5,
> > from ./arch/x86/include/asm/thread_info.h:53,
> > from ./include/linux/thread_info.h:60,
> > from ./arch/x86/include/asm/preempt.h:9,
> > from ./include/linux/preempt.h:79,
> > from ./include/linux/spinlock.h:56,
> > from ./include/linux/mmzone.h:8,
> > from ./include/linux/gfp.h:7,
> > from ./include/linux/slab.h:16,
> > from ./include/linux/resource_ext.h:11,
> > from ./include/linux/acpi.h:13,
> > from drivers/pnp/pnpacpi/rsparser.c:11:
> > In function 'fortify_memcpy_chk',
> > inlined from 'pnpacpi_parse_allocated_vendor' at drivers/pnp/pnpacpi/rsparser.c:158:3,
> > inlined from 'pnpacpi_allocated_resource' at drivers/pnp/pnpacpi/rsparser.c:249:3:
> > ./include/linux/fortify-string.h:588:25: warning: call to '__read_overflow2_field'
> > declared with attribute warning: detected read beyond size of field (2nd parameter);
> > maybe use struct_group()? [-Wattribute-warning]
> > 588 | __read_overflow2_field(q_size_field, size);
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > According to the comments in include/linux/fortify-string.h, 'memcpy()',
> > 'memmove()' and 'memset()' must not be used beyond individual struct
> > members to ensure that the compiler can enforce protection against
> > buffer overflows, and, IIUC, this also applies to partial copies from
> > the particular member ('vendor->byte_data' in this case). So it should
> > be better (and safer) to do both copies at once (and 'byte_data' of
> > 'struct acpi_resource_vendor_typed' seems to be a good candidate for
> > '__counted_by(byte_length)' as well).
> >
> > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> > ---
> > v2: prefer sizeof(range) over hardcoded constant (Rafael J. Wysocki)
>
> Yeah, this looks good to me. Thanks for the fix!
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
Applied as 6.8 material, thanks!
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-12-06 20:04 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-27 14:58 [PATCH] pnp: acpi: fix fortify warning Dmitry Antipov
2023-11-27 19:01 ` Rafael J. Wysocki
2023-11-28 2:52 ` [PATCH] [v2] " Dmitry Antipov
2023-11-30 23:29 ` Kees Cook
2023-12-06 20:03 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox