diff mbox series

[06/11] acpi: avoid warning for truncated string copy

Message ID 20240328140512.4148825-7-arnd@kernel.org (mailing list archive)
State Changes Requested, archived
Headers show
Series address remaining stringop-truncation warnings | expand

Commit Message

Arnd Bergmann March 28, 2024, 2:04 p.m. UTC 
From: Arnd Bergmann <arnd@arndb.de>

gcc -Wstringop-truncation warns about copying a string that results in a
missing nul termination:

drivers/acpi/acpica/tbfind.c: In function 'acpi_tb_find_table':
drivers/acpi/acpica/tbfind.c:60:9: error: 'strncpy' specified bound 6 equals destination size [-Werror=stringop-truncation]
   60 |         strncpy(header.oem_id, oem_id, ACPI_OEM_ID_SIZE);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/acpi/acpica/tbfind.c:61:9: error: 'strncpy' specified bound 8 equals destination size [-Werror=stringop-truncation]
   61 |         strncpy(header.oem_table_id, oem_table_id, ACPI_OEM_TABLE_ID_SIZE);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This one is intentional, so rewrite the code in a way that avoids the
warning. Since there is already an extra strlen() and an overflow check,
the actual size to be copied is already known here.

Fixes: 47c08729bf1c ("ACPICA: Fix for LoadTable operator, input strings")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/acpi/acpica/tbfind.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

Comments

Justin Stitt March 28, 2024, 11:20 p.m. UTC | #1 
Hi,

On Thu, Mar 28, 2024 at 03:04:50PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> gcc -Wstringop-truncation warns about copying a string that results in a
> missing nul termination:
> 
> drivers/acpi/acpica/tbfind.c: In function 'acpi_tb_find_table':
> drivers/acpi/acpica/tbfind.c:60:9: error: 'strncpy' specified bound 6 equals destination size [-Werror=stringop-truncation]
>    60 |         strncpy(header.oem_id, oem_id, ACPI_OEM_ID_SIZE);
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/acpi/acpica/tbfind.c:61:9: error: 'strncpy' specified bound 8 equals destination size [-Werror=stringop-truncation]
>    61 |         strncpy(header.oem_table_id, oem_table_id, ACPI_OEM_TABLE_ID_SIZE);
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> This one is intentional, so rewrite the code in a way that avoids the
> warning. Since there is already an extra strlen() and an overflow check,
> the actual size to be copied is already known here.
>
I also tried cleaning these up but Kees informed me that this subsystem
is maintained elsewhere:

https://lore.kernel.org/all/202308241612.DFE4119@keescook/

I am not sure if you can get changes through by the traditional means.

> 
> Fixes: 47c08729bf1c ("ACPICA: Fix for LoadTable operator, input strings")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/acpi/acpica/tbfind.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/acpi/acpica/tbfind.c b/drivers/acpi/acpica/tbfind.c
> index 1c1b2e284bd9..472ce2a6624b 100644
> --- a/drivers/acpi/acpica/tbfind.c
> +++ b/drivers/acpi/acpica/tbfind.c
> @@ -36,7 +36,7 @@ acpi_tb_find_table(char *signature,
>  {
>  	acpi_status status = AE_OK;
>  	struct acpi_table_header header;
> -	u32 i;
> +	u32 len, i;
>  
>  	ACPI_FUNCTION_TRACE(tb_find_table);
>  
> @@ -46,19 +46,18 @@ acpi_tb_find_table(char *signature,
>  		return_ACPI_STATUS(AE_BAD_SIGNATURE);
>  	}
>  
> -	/* Don't allow the OEM strings to be too long */
> -
> -	if ((strlen(oem_id) > ACPI_OEM_ID_SIZE) ||
> -	    (strlen(oem_table_id) > ACPI_OEM_TABLE_ID_SIZE)) {
> -		return_ACPI_STATUS(AE_AML_STRING_LIMIT);
> -	}
> -
>  	/* Normalize the input strings */
>  
>  	memset(&header, 0, sizeof(struct acpi_table_header));
>  	ACPI_COPY_NAMESEG(header.signature, signature);
> -	strncpy(header.oem_id, oem_id, ACPI_OEM_ID_SIZE);
> -	strncpy(header.oem_table_id, oem_table_id, ACPI_OEM_TABLE_ID_SIZE);
> +	len = strlen(oem_id);
> +	if (len > ACPI_OEM_ID_SIZE)
> +		return_ACPI_STATUS(AE_AML_STRING_LIMIT);
> +	memcpy(header.oem_id, oem_id, len);
> +	len = strlen(oem_table_id);
> +	if (len > ACPI_OEM_TABLE_ID_SIZE)
> +		return_ACPI_STATUS(AE_AML_STRING_LIMIT);
> +	memcpy(header.oem_table_id, oem_table_id, len);
>  
>  	/* Search for the table */
>  
> -- 
> 2.39.2
> 
Thanks
Justin
Rafael J. Wysocki April 8, 2024, 2:41 p.m. UTC | #2 
On Thu, Mar 28, 2024 at 3:06 PM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> gcc -Wstringop-truncation warns about copying a string that results in a
> missing nul termination:
>
> drivers/acpi/acpica/tbfind.c: In function 'acpi_tb_find_table':
> drivers/acpi/acpica/tbfind.c:60:9: error: 'strncpy' specified bound 6 equals destination size [-Werror=stringop-truncation]
>    60 |         strncpy(header.oem_id, oem_id, ACPI_OEM_ID_SIZE);
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/acpi/acpica/tbfind.c:61:9: error: 'strncpy' specified bound 8 equals destination size [-Werror=stringop-truncation]
>    61 |         strncpy(header.oem_table_id, oem_table_id, ACPI_OEM_TABLE_ID_SIZE);
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> This one is intentional, so rewrite the code in a way that avoids the
> warning. Since there is already an extra strlen() and an overflow check,
> the actual size to be copied is already known here.
>
> Fixes: 47c08729bf1c ("ACPICA: Fix for LoadTable operator, input strings")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Because ACPICA is an external project supplying code to the Linux
kernel, the way to change the ACPICA code in the kernel is to submit a
pull request to the upstream ACPICA project on GitHub and once that PR
has been merged, submit a Linux patch corresponding to it including
the Link: tag pointing to the PR in question and the git ID of the
corresponding upstream ACPICA commit.

However, note that upstream ACPICA changes are applied to the Linux
kernel source code every time the upstream ACPICA project makes a
release, so it is not necessary to send the corresponding Linux
patches for them unless in the cases when timing matters.

> ---
>  drivers/acpi/acpica/tbfind.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/acpi/acpica/tbfind.c b/drivers/acpi/acpica/tbfind.c
> index 1c1b2e284bd9..472ce2a6624b 100644
> --- a/drivers/acpi/acpica/tbfind.c
> +++ b/drivers/acpi/acpica/tbfind.c
> @@ -36,7 +36,7 @@ acpi_tb_find_table(char *signature,
>  {
>         acpi_status status = AE_OK;
>         struct acpi_table_header header;
> -       u32 i;
> +       u32 len, i;
>
>         ACPI_FUNCTION_TRACE(tb_find_table);
>
> @@ -46,19 +46,18 @@ acpi_tb_find_table(char *signature,
>                 return_ACPI_STATUS(AE_BAD_SIGNATURE);
>         }
>
> -       /* Don't allow the OEM strings to be too long */
> -
> -       if ((strlen(oem_id) > ACPI_OEM_ID_SIZE) ||
> -           (strlen(oem_table_id) > ACPI_OEM_TABLE_ID_SIZE)) {
> -               return_ACPI_STATUS(AE_AML_STRING_LIMIT);
> -       }
> -
>         /* Normalize the input strings */
>
>         memset(&header, 0, sizeof(struct acpi_table_header));
>         ACPI_COPY_NAMESEG(header.signature, signature);
> -       strncpy(header.oem_id, oem_id, ACPI_OEM_ID_SIZE);
> -       strncpy(header.oem_table_id, oem_table_id, ACPI_OEM_TABLE_ID_SIZE);
> +       len = strlen(oem_id);
> +       if (len > ACPI_OEM_ID_SIZE)
> +               return_ACPI_STATUS(AE_AML_STRING_LIMIT);
> +       memcpy(header.oem_id, oem_id, len);
> +       len = strlen(oem_table_id);
> +       if (len > ACPI_OEM_TABLE_ID_SIZE)
> +               return_ACPI_STATUS(AE_AML_STRING_LIMIT);
> +       memcpy(header.oem_table_id, oem_table_id, len);
>
>         /* Search for the table */
>
> --
> 2.39.2
>
>
diff mbox series

Patch

diff --git a/drivers/acpi/acpica/tbfind.c b/drivers/acpi/acpica/tbfind.c
index 1c1b2e284bd9..472ce2a6624b 100644
--- a/drivers/acpi/acpica/tbfind.c
+++ b/drivers/acpi/acpica/tbfind.c
@@ -36,7 +36,7 @@  acpi_tb_find_table(char *signature,
 {
 	acpi_status status = AE_OK;
 	struct acpi_table_header header;
-	u32 i;
+	u32 len, i;
 
 	ACPI_FUNCTION_TRACE(tb_find_table);
 
@@ -46,19 +46,18 @@  acpi_tb_find_table(char *signature,
 		return_ACPI_STATUS(AE_BAD_SIGNATURE);
 	}
 
-	/* Don't allow the OEM strings to be too long */
-
-	if ((strlen(oem_id) > ACPI_OEM_ID_SIZE) ||
-	    (strlen(oem_table_id) > ACPI_OEM_TABLE_ID_SIZE)) {
-		return_ACPI_STATUS(AE_AML_STRING_LIMIT);
-	}
-
 	/* Normalize the input strings */
 
 	memset(&header, 0, sizeof(struct acpi_table_header));
 	ACPI_COPY_NAMESEG(header.signature, signature);
-	strncpy(header.oem_id, oem_id, ACPI_OEM_ID_SIZE);
-	strncpy(header.oem_table_id, oem_table_id, ACPI_OEM_TABLE_ID_SIZE);
+	len = strlen(oem_id);
+	if (len > ACPI_OEM_ID_SIZE)
+		return_ACPI_STATUS(AE_AML_STRING_LIMIT);
+	memcpy(header.oem_id, oem_id, len);
+	len = strlen(oem_table_id);
+	if (len > ACPI_OEM_TABLE_ID_SIZE)
+		return_ACPI_STATUS(AE_AML_STRING_LIMIT);
+	memcpy(header.oem_table_id, oem_table_id, len);
 
 	/* Search for the table */