linux-kernel - Re: [PATCH] platform/x86: thinkpad_acpi: replace deprecated strncpy with memcpy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <c0410813-d8cc-4609-92ba-ec41d107e99d@app.fastmail.com>
Date:   Sat, 21 Oct 2023 16:55:37 -0400
From:   "Mark Pearson" <mpearson-lenovo@...ebb.ca>
To:     "Justin Stitt" <justinstitt@...gle.com>,
        "Kees Cook" <keescook@...omium.org>,
        "Henrique de Moraes Holschuh" <hmh@....eng.br>,
        "Hans de Goede" <hdegoede@...hat.com>,
        Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        "markgross@...nel.org" <markgross@...nel.org>
Cc:     ibm-acpi-devel@...ts.sourceforge.net,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] platform/x86: thinkpad_acpi: replace deprecated strncpy with
 memcpy

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@...omium.org>
> Signed-off-by: Justin Stitt <justinstitt@...gle.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@...gle.com>

Looks good to me.
Reviewed-by: Mark Pearson <mpearson-lenovo@...ebb.ca>