linux-kernel - [PATCH v4] staging: vme_user: replace strcpy with strscpy

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <ZTDS2H48JBUTiwZi@lab-ubuntu>
Date:   Thu, 19 Oct 2023 09:55:20 +0300
From:   Calvince Otieno <calvncce@...il.com>
To:     gustavo@...eddedor.com, outreachy@...ts.linux.dev
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        Dan Carpenter <dan.carpenter@...aro.org>
Subject: [PATCH v4] staging: vme_user: replace strcpy with strscpy

Checkpatch suggests using strscpy() instead of strcpy().

The advantages of strscpy() are that it always adds a NUL terminator
and prevents read/write overflows if the source string is not properly
terminated.

strcpy() lacks built-in bounds checking for the destination buffer,
making it susceptible to buffer overflows. These overflows can lead
to various unpredictable behaviors.

In this specific context, both strscpy and strcpy performs the same
operation without any functional difference.

The reason for this equivalence is that the driver_name string "vme_fake"
is shorter than the size of the fake_bridge->name array which is defined
as 16 characters (struct vme_bridge {char name[VMENAMSIZ];...}). Thus,
there is no risk of buffer overflow in either case. VMENAMSIZ variable
holds a constant value of 16 (#define VMENAMSIZ 16)

The null-terminated "vme_fake" string
(static const char driver_name[] = "vme_fake";) can be safely copied into
fake_bridge->name using either strscpy or strcpy.

While using strscpy() does not address any bugs, it is considered a better
practice and aligns with checkpatch recommendations.

Reviewed-by: Dan Carpenter <dan.carpenter@...aro.org>
Signed-off-by: Calvince Otieno <calvncce@...il.com>
---
Changes in v4:
 - Added a semicolon to mark the end of the strscpy() implementation.
 The semicolon got deleted accidentally while mailing the previous patch
 versions.
 - Added the patch reviewer.
---
Changes in v3:
 - Rewrote the commit message
---
Changes in v2:
 - Make the commit message more clearer.
 - Remove the incident extra line change.

---
 drivers/staging/vme_user/vme_fake.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vme_user/vme_fake.c b/drivers/staging/vme_user/vme_fake.c
index 0e02c194298d..8ab5b43c70a6 100644
--- a/drivers/staging/vme_user/vme_fake.c
+++ b/drivers/staging/vme_user/vme_fake.c
@@ -1091,7 +1091,7 @@ static int __init fake_init(void)
 	tasklet_init(&fake_device->int_tasklet, fake_VIRQ_tasklet,
 		     (unsigned long)fake_bridge);

-	strcpy(fake_bridge->name, driver_name);
+	strscpy(fake_bridge->name, driver_name, sizeof(fake_bridge->name));

 	/* Add master windows to list */
 	INIT_LIST_HEAD(&fake_bridge->master_resources);
-- 
2.34.1