diff mbox series

platform/x86: thinkpad_acpi: replace deprecated strncpy with memcpy

Message ID 20231020-strncpy-drivers-platform-x86-thinkpad_acpi-c-v1-1-312f2e33034f@google.com (mailing list archive)
State Accepted, archived
Headers show
Series platform/x86: thinkpad_acpi: replace deprecated strncpy with memcpy | expand

Commit Message

Justin Stitt Oct. 20, 2023, 5:52 p.m. UTC 
strncpy() is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous
interfaces.

We expect ec_fw_string to be NUL-terminated based on its use with format
strings in thinkpad_acpi.c:
11241 | pr_notice("ThinkPad firmware release %s doesn't match the known patterns\n",
11242 |     ec_fw_string);

Moreover, NUL-padding is not required since ec_fw_string is explicitly
zero-initialized:
11185 | char ec_fw_string[18] = {0};

When carefully copying bytes from one buffer to another in
pre-determined blocks (like what's happening here with dmi_data):

|       static void find_new_ec_fwstr(const struct dmi_header *dm, void *private)
|       {
|       	char *ec_fw_string = (char *) private;
|       	const char *dmi_data = (const char *)dm;
|       	/*
|       	 * ThinkPad Embedded Controller Program Table on newer models
|       	 *
|       	 * Offset |  Name                | Width  | Description
|       	 * ----------------------------------------------------
|       	 *  0x00  | Type                 | BYTE   | 0x8C
|       	 *  0x01  | Length               | BYTE   |
|       	 *  0x02  | Handle               | WORD   | Varies
|       	 *  0x04  | Signature            | BYTEx6 | ASCII for "LENOVO"
|       	 *  0x0A  | OEM struct offset    | BYTE   | 0x0B
|       	 *  0x0B  | OEM struct number    | BYTE   | 0x07, for this structure
|       	 *  0x0C  | OEM struct revision  | BYTE   | 0x01, for this format
|       	 *  0x0D  | ECP version ID       | STR ID |
|       	 *  0x0E  | ECP release date     | STR ID |
|       	 */
|
|       	/* Return if data structure not match */
|       	if (dm->type != 140 || dm->length < 0x0F ||
|       	memcmp(dmi_data + 4, "LENOVO", 6) != 0 ||
|       	dmi_data[0x0A] != 0x0B || dmi_data[0x0B] != 0x07 ||
|       	dmi_data[0x0C] != 0x01)
|       		return;
|
|       	/* fwstr is the first 8byte string  */
|       	strncpy(ec_fw_string, dmi_data + 0x0F, 8);

... we shouldn't be using a C string api. Let's instead use memcpy() as
this more properly relays the intended behavior.

Do note that ec_fw_string will still end up being NUL-terminated since
we are memcpy'ing only 8 bytes into a buffer full of 18 zeroes. There's
still some trailing NUL-bytes there. To ensure this behavior, let's add
a BUILD_BUG_ON checking the length leaves space for at least one
trailing NUL-byte.

Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://github.com/KSPP/linux/issues/90
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Note: build-tested only.

Found with: $ rg "strncpy\("
---
 drivers/platform/x86/thinkpad_acpi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)


---
base-commit: dab3e01664eaddae965699f1fec776609db0ea9d
change-id: 20231019-strncpy-drivers-platform-x86-thinkpad_acpi-c-7a733d087ef7

Best regards,
--
Justin Stitt <justinstitt@google.com>

Comments

Kees Cook Oct. 20, 2023, 5:55 p.m. UTC | #1 
On Fri, Oct 20, 2023 at 05:52:43PM +0000, Justin Stitt wrote:
> strncpy() is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous
> interfaces.
> 
> We expect ec_fw_string to be NUL-terminated based on its use with format
> strings in thinkpad_acpi.c:
> 11241 | pr_notice("ThinkPad firmware release %s doesn't match the known patterns\n",
> 11242 |     ec_fw_string);
> 
> Moreover, NUL-padding is not required since ec_fw_string is explicitly
> zero-initialized:
> 11185 | char ec_fw_string[18] = {0};
> 
> When carefully copying bytes from one buffer to another in
> pre-determined blocks (like what's happening here with dmi_data):
> 
> |       static void find_new_ec_fwstr(const struct dmi_header *dm, void *private)
> |       {
> |       	char *ec_fw_string = (char *) private;
> |       	const char *dmi_data = (const char *)dm;
> |       	/*
> |       	 * ThinkPad Embedded Controller Program Table on newer models
> |       	 *
> |       	 * Offset |  Name                | Width  | Description
> |       	 * ----------------------------------------------------
> |       	 *  0x00  | Type                 | BYTE   | 0x8C
> |       	 *  0x01  | Length               | BYTE   |
> |       	 *  0x02  | Handle               | WORD   | Varies
> |       	 *  0x04  | Signature            | BYTEx6 | ASCII for "LENOVO"
> |       	 *  0x0A  | OEM struct offset    | BYTE   | 0x0B
> |       	 *  0x0B  | OEM struct number    | BYTE   | 0x07, for this structure
> |       	 *  0x0C  | OEM struct revision  | BYTE   | 0x01, for this format
> |       	 *  0x0D  | ECP version ID       | STR ID |
> |       	 *  0x0E  | ECP release date     | STR ID |
> |       	 */
> |
> |       	/* Return if data structure not match */
> |       	if (dm->type != 140 || dm->length < 0x0F ||
> |       	memcmp(dmi_data + 4, "LENOVO", 6) != 0 ||
> |       	dmi_data[0x0A] != 0x0B || dmi_data[0x0B] != 0x07 ||
> |       	dmi_data[0x0C] != 0x01)
> |       		return;
> |
> |       	/* fwstr is the first 8byte string  */
> |       	strncpy(ec_fw_string, dmi_data + 0x0F, 8);
> 
> ... we shouldn't be using a C string api. Let's instead use memcpy() as
> this more properly relays the intended behavior.
> 
> Do note that ec_fw_string will still end up being NUL-terminated since
> we are memcpy'ing only 8 bytes into a buffer full of 18 zeroes. There's
> still some trailing NUL-bytes there. To ensure this behavior, let's add
> a BUILD_BUG_ON checking the length leaves space for at least one
> trailing NUL-byte.
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Justin Stitt <justinstitt@google.com>

This looks like the best choice given how this code is designed. The
"char *private" prototype is weird since there's only one user, but
okay. :P

Reviewed-by: Kees Cook <keescook@chromium.org>
Mark Pearson Oct. 21, 2023, 8:55 p.m. UTC | #2 
On Fri, Oct 20, 2023, at 1:52 PM, Justin Stitt wrote:
> strncpy() is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous
> interfaces.
>
> We expect ec_fw_string to be NUL-terminated based on its use with format
> strings in thinkpad_acpi.c:
> 11241 | pr_notice("ThinkPad firmware release %s doesn't match the known 
> patterns\n",
> 11242 |     ec_fw_string);
>
> Moreover, NUL-padding is not required since ec_fw_string is explicitly
> zero-initialized:
> 11185 | char ec_fw_string[18] = {0};
>
> When carefully copying bytes from one buffer to another in
> pre-determined blocks (like what's happening here with dmi_data):
>
> |       static void find_new_ec_fwstr(const struct dmi_header *dm, void 
> *private)
> |       {
> |       	char *ec_fw_string = (char *) private;
> |       	const char *dmi_data = (const char *)dm;
> |       	/*
> |       	 * ThinkPad Embedded Controller Program Table on newer models
> |       	 *
> |       	 * Offset |  Name                | Width  | Description
> |       	 * ----------------------------------------------------
> |       	 *  0x00  | Type                 | BYTE   | 0x8C
> |       	 *  0x01  | Length               | BYTE   |
> |       	 *  0x02  | Handle               | WORD   | Varies
> |       	 *  0x04  | Signature            | BYTEx6 | ASCII for "LENOVO"
> |       	 *  0x0A  | OEM struct offset    | BYTE   | 0x0B
> |       	 *  0x0B  | OEM struct number    | BYTE   | 0x07, for this 
> structure
> |       	 *  0x0C  | OEM struct revision  | BYTE   | 0x01, for this 
> format
> |       	 *  0x0D  | ECP version ID       | STR ID |
> |       	 *  0x0E  | ECP release date     | STR ID |
> |       	 */
> |
> |       	/* Return if data structure not match */
> |       	if (dm->type != 140 || dm->length < 0x0F ||
> |       	memcmp(dmi_data + 4, "LENOVO", 6) != 0 ||
> |       	dmi_data[0x0A] != 0x0B || dmi_data[0x0B] != 0x07 ||
> |       	dmi_data[0x0C] != 0x01)
> |       		return;
> |
> |       	/* fwstr is the first 8byte string  */
> |       	strncpy(ec_fw_string, dmi_data + 0x0F, 8);
>
> ... we shouldn't be using a C string api. Let's instead use memcpy() as
> this more properly relays the intended behavior.
>
> Do note that ec_fw_string will still end up being NUL-terminated since
> we are memcpy'ing only 8 bytes into a buffer full of 18 zeroes. There's
> still some trailing NUL-bytes there. To ensure this behavior, let's add
> a BUILD_BUG_ON checking the length leaves space for at least one
> trailing NUL-byte.
>
> Link: 
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings 
> [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> Note: build-tested only.
>
> Found with: $ rg "strncpy\("
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c 
> b/drivers/platform/x86/thinkpad_acpi.c
> index 41584427dc32..bd9e06f5b860 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -11144,6 +11144,8 @@ static char __init tpacpi_parse_fw_id(const 
> char * const s,
>  	return '\0';
>  }
> 
> +#define EC_FW_STRING_LEN 18
> +
>  static void find_new_ec_fwstr(const struct dmi_header *dm, void 
> *private)
>  {
>  	char *ec_fw_string = (char *) private;
> @@ -11172,7 +11174,8 @@ static void find_new_ec_fwstr(const struct 
> dmi_header *dm, void *private)
>  		return;
> 
>  	/* fwstr is the first 8byte string  */
> -	strncpy(ec_fw_string, dmi_data + 0x0F, 8);
> +	BUILD_BUG_ON(EC_FW_STRING_LEN <= 8);
> +	memcpy(ec_fw_string, dmi_data + 0x0F, 8);
>  }
> 
>  /* returns 0 - probe ok, or < 0 - probe error.
> @@ -11182,7 +11185,7 @@ static int __must_check __init get_thinkpad_model_data(
>  						struct thinkpad_id_data *tp)
>  {
>  	const struct dmi_device *dev = NULL;
> -	char ec_fw_string[18] = {0};
> +	char ec_fw_string[EC_FW_STRING_LEN] = {0};
>  	char const *s;
>  	char t;
> 
>
> ---
> base-commit: dab3e01664eaddae965699f1fec776609db0ea9d
> change-id: 20231019-strncpy-drivers-platform-x86-thinkpad_acpi-c-7a733d087ef7
>
> Best regards,
> --
> Justin Stitt <justinstitt@google.com>

Looks good to me.
Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Ilpo Järvinen Oct. 23, 2023, 2:02 p.m. UTC | #3 
On Fri, 20 Oct 2023 17:52:43 +0000, Justin Stitt wrote:

> strncpy() is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous
> interfaces.
> 
> We expect ec_fw_string to be NUL-terminated based on its use with format
> strings in thinkpad_acpi.c:
> 11241 | pr_notice("ThinkPad firmware release %s doesn't match the known patterns\n",
> 11242 |     ec_fw_string);
> 
> [...]


Thank you for your contribution, it has been applied to my local
review-ilpo branch. Note it will show up in the public
platform-drivers-x86/review-ilpo branch only once I've pushed my
local branch there, which might take a while.

Once I've run some tests on the review-ilpo branch the patches
there will be added to the platform-drivers-x86/for-next branch
and eventually will be included in the pdx86 pull-request to
Linus for the next merge-window.

The list of commits applied:
[1/1] platform/x86: thinkpad_acpi: replace deprecated strncpy with memcpy
      commit: 942a4a61b64e182d756a1a5776aa500d3b3d862f

--
 i.
diff mbox series

Patch

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 41584427dc32..bd9e06f5b860 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -11144,6 +11144,8 @@  static char __init tpacpi_parse_fw_id(const char * const s,
 	return '\0';
 }
 
+#define EC_FW_STRING_LEN 18
+
 static void find_new_ec_fwstr(const struct dmi_header *dm, void *private)
 {
 	char *ec_fw_string = (char *) private;
@@ -11172,7 +11174,8 @@  static void find_new_ec_fwstr(const struct dmi_header *dm, void *private)
 		return;
 
 	/* fwstr is the first 8byte string  */
-	strncpy(ec_fw_string, dmi_data + 0x0F, 8);
+	BUILD_BUG_ON(EC_FW_STRING_LEN <= 8);
+	memcpy(ec_fw_string, dmi_data + 0x0F, 8);
 }
 
 /* returns 0 - probe ok, or < 0 - probe error.
@@ -11182,7 +11185,7 @@  static int __must_check __init get_thinkpad_model_data(
 						struct thinkpad_id_data *tp)
 {
 	const struct dmi_device *dev = NULL;
-	char ec_fw_string[18] = {0};
+	char ec_fw_string[EC_FW_STRING_LEN] = {0};
 	char const *s;
 	char t;