Re: [PATCH] [v2] pnp: acpi: fix fortify warning

[Kees Cook <keescook@chromium.org>]

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