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 <[email protected]>
Signed-off-by: Justin Stitt <[email protected]>
---
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 <[email protected]>
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 <[email protected]>
> Signed-off-by: Justin Stitt <[email protected]>
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 <[email protected]>
--
Kees Cook
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 <[email protected]>
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> 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 <[email protected]>
Looks good to me.
Reviewed-by: Mark Pearson <[email protected]>
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.