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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx>
-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
