diff mbox series

[RFC] ACPICA: remove acpi_ut_safe_strncpy in favor of strscpy

Message ID 20230824-strncpy-drivers-acpi-acpica-v1-1-d027ba183b66@google.com (mailing list archive)
State RFC, archived
Headers show
Series [RFC] ACPICA: remove acpi_ut_safe_strncpy in favor of strscpy | expand

Commit Message

Justin Stitt Aug. 24, 2023, 10:02 p.m. UTC
I wanted to gather some thoughts on removing `acpi_ut_safe_strncpy` (and
potentially other `acpi...safe...()` interfaces) in favor of
pre-existing interfaces in the kernel (like strscpy).

Running a git blame shows these functions were implemented 10 years ago
and their implementations generally mirror the _newer_ and more robust
stuff in lib/string.h -- Let's just use these, right?

I appreciate any comments and whether or not I should stop at just
`strncpy`.

Thanks

Signed-off-by: Justin Stitt <justinstitt@google.com>
---
 drivers/acpi/acpica/acutils.h   | 2 --
 drivers/acpi/acpica/dbfileio.c  | 2 +-
 drivers/acpi/acpica/psutils.c   | 2 +-
 drivers/acpi/acpica/utnonansi.c | 8 --------
 drivers/acpi/acpica/uttrack.c   | 2 +-
 5 files changed, 3 insertions(+), 13 deletions(-)


---
base-commit: f9604036a3fb6149badf346994b46b03f9292db7
change-id: 20230824-strncpy-drivers-acpi-acpica-321af3eb414e

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

Comments

Kees Cook Aug. 24, 2023, 11:21 p.m. UTC | #1
On Thu, Aug 24, 2023 at 10:02:02PM +0000, Justin Stitt wrote:
> I wanted to gather some thoughts on removing `acpi_ut_safe_strncpy` (and
> potentially other `acpi...safe...()` interfaces) in favor of
> pre-existing interfaces in the kernel (like strscpy).
> 
> Running a git blame shows these functions were implemented 10 years ago
> and their implementations generally mirror the _newer_ and more robust
> stuff in lib/string.h -- Let's just use these, right?
> 
> I appreciate any comments and whether or not I should stop at just
> `strncpy`.

ACPICA is actually a separate upstream project, so changes are best made
there[1]. However, this code base is shared with many OSes and
compilers, so there won't be a common "strscpy" available. Perhaps the
right thing to do here is to implement acpi_ut_safe_strncpy() in terms
of strnlen(), memcpy(), and memset(). That would make the upstream
project safe against "too long reads", etc, and would require no
collateral changes:

void acpi_ut_safe_strncpy(char *dest, char *source, acpi_size dest_size)
{
	/* Do not over-read the source string. */
	acpi_size len = 0;

	if (dest_size > 0)
		len = strnlen(source, dest_size - 1);
	if (len)
		memcpy(dest, source, len)
	/* Always terminate destination string and pad to dest_size. */
	memset(dest + len, '\0', dest_size - len);
}

-Kees

[1] https://github.com/acpica/acpica
    e.g. https://github.com/acpica/acpica/pull/856
diff mbox series

Patch

diff --git a/drivers/acpi/acpica/acutils.h b/drivers/acpi/acpica/acutils.h
index edfdbbef81c1..4d5c401a14b4 100644
--- a/drivers/acpi/acpica/acutils.h
+++ b/drivers/acpi/acpica/acutils.h
@@ -626,8 +626,6 @@  void acpi_ut_repair_name(char *name);
 #if defined (ACPI_DEBUGGER) || defined (ACPI_APPLICATION) || defined (ACPI_DEBUG_OUTPUT)
 u8 acpi_ut_safe_strcpy(char *dest, acpi_size dest_size, char *source);
 
-void acpi_ut_safe_strncpy(char *dest, char *source, acpi_size dest_size);
-
 u8 acpi_ut_safe_strcat(char *dest, acpi_size dest_size, char *source);
 
 u8
diff --git a/drivers/acpi/acpica/dbfileio.c b/drivers/acpi/acpica/dbfileio.c
index e1b6e54a96ac..197e1ff1f72c 100644
--- a/drivers/acpi/acpica/dbfileio.c
+++ b/drivers/acpi/acpica/dbfileio.c
@@ -63,7 +63,7 @@  void acpi_db_open_debug_file(char *name)
 	}
 
 	acpi_os_printf("Debug output file %s opened\n", name);
-	acpi_ut_safe_strncpy(acpi_gbl_db_debug_filename, name,
+	strscpy(acpi_gbl_db_debug_filename, name,
 			     sizeof(acpi_gbl_db_debug_filename));
 	acpi_gbl_db_output_to_file = TRUE;
 }
diff --git a/drivers/acpi/acpica/psutils.c b/drivers/acpi/acpica/psutils.c
index d550c4af4702..95af2c74e00b 100644
--- a/drivers/acpi/acpica/psutils.c
+++ b/drivers/acpi/acpica/psutils.c
@@ -60,7 +60,7 @@  void acpi_ps_init_op(union acpi_parse_object *op, u16 opcode)
 	op->common.descriptor_type = ACPI_DESC_TYPE_PARSER;
 	op->common.aml_opcode = opcode;
 
-	ACPI_DISASM_ONLY_MEMBERS(acpi_ut_safe_strncpy(op->common.aml_op_name,
+	ACPI_DISASM_ONLY_MEMBERS(strscpy(op->common.aml_op_name,
 						      (acpi_ps_get_opcode_info
 						       (opcode))->name,
 						      sizeof(op->common.
diff --git a/drivers/acpi/acpica/utnonansi.c b/drivers/acpi/acpica/utnonansi.c
index ff0802ace19b..a465e5a1d309 100644
--- a/drivers/acpi/acpica/utnonansi.c
+++ b/drivers/acpi/acpica/utnonansi.c
@@ -164,12 +164,4 @@  acpi_ut_safe_strncat(char *dest,
 	return (FALSE);
 }
 
-void acpi_ut_safe_strncpy(char *dest, char *source, acpi_size dest_size)
-{
-	/* Always terminate destination string */
-
-	strncpy(dest, source, dest_size);
-	dest[dest_size - 1] = 0;
-}
-
 #endif
diff --git a/drivers/acpi/acpica/uttrack.c b/drivers/acpi/acpica/uttrack.c
index f5f5da441458..a96a86bc5781 100644
--- a/drivers/acpi/acpica/uttrack.c
+++ b/drivers/acpi/acpica/uttrack.c
@@ -368,7 +368,7 @@  acpi_ut_track_allocation(struct acpi_debug_mem_block *allocation,
 	allocation->component = component;
 	allocation->line = line;
 
-	acpi_ut_safe_strncpy(allocation->module, (char *)module,
+	strscpy(allocation->module, (char *)module,
 			     ACPI_MAX_MODULE_NAME);
 
 	if (!element) {