[PATCH next] drivers/rpmsg: Fix copy of channel->name into open request

[PATCH next] drivers/rpmsg: Fix copy of channel->name into open request

[david.laight.linux]

From: David Laight <david.laight.linux@gmail.com>

Nothing obvious ensures that the name is less than GLINK_CMD_OPEN (32)
bytes long. It is specified by the external caller.
The constant is only used in this function.

Truncate longer names to avoid smashing the stack.
zero-pad the copy to avoid sending stale stack bytes to the
remote system.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---
This is one of a group of patches that remove potentially unbounded
strcpy() calls.

They are mostly replaced by strscpy() or, when strlen() has just been
called, with memcpy() (usually including the '\0').

Calls with copy string literals into arrays are left unchanged.
They are safe and easily detected as such.

The changes were made by getting the compiler to detect the calls and
then fixing the code by hand.

Note that all the changes are only compile tested.

Some Makefiles were changed to allow files to contain strcpy().
As well as 'difficult to fix' files, this included 'show' functions
as they really need to use sysfs_emit() or seq_printf().

All the patches are being sent individually to avoid very long cc lists.
Apologies for the terse commit messages and likely unexpected tags.
(There are about 100 patches in total.)

 drivers/rpmsg/qcom_glink_native.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 401a4ece0c97..d10abab6881e 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -481,8 +481,7 @@ static int qcom_glink_send_open_req(struct qcom_glink *glink,
 				    struct glink_channel *channel)
 {
 	DEFINE_RAW_FLEX(struct glink_msg, req, data, GLINK_NAME_SIZE);
-	int name_len = strlen(channel->name) + 1;
-	int req_len = ALIGN(sizeof(*req) + name_len, 8);
+	int name_len, req_len;
 	int ret;
 	unsigned long flags;
 
@@ -498,14 +497,20 @@ static int qcom_glink_send_open_req(struct qcom_glink *glink,
 
 	channel->lcid = ret;
 
+	name_len = strscpy_pad(req->data, channel->name, GLINK_NAME_SIZE);
+	if (name_len < 0)
+		name_len = GLINK_NAME_SIZE;
+	else
+		name_len++;
+
 	req->cmd = cpu_to_le16(GLINK_CMD_OPEN);
 	req->param1 = cpu_to_le16(channel->lcid);
 	req->param2 = cpu_to_le32(name_len);
-	strcpy(req->data, channel->name);
 
 	trace_qcom_glink_cmd_open_tx(glink->label, channel->name,
 				     channel->lcid, channel->rcid);
 
+	req_len = ALIGN(sizeof(*req) + name_len, 8);
 	ret = qcom_glink_tx(glink, req, req_len, NULL, 0, true);
 	if (ret)
 		goto remove_idr;
-- 
2.39.5

Re: [PATCH next] drivers/rpmsg: Fix copy of channel->name into open request

[Konrad Dybcio]

On 6/8/26 11:55 AM, david.laight.linux@gmail.com wrote:
> From: David Laight <david.laight.linux@gmail.com>
> 
> Nothing obvious ensures that the name is less than GLINK_CMD_OPEN (32)
						     ^ GLINK_NAME_SIZE


[...]

> @@ -481,8 +481,7 @@ static int qcom_glink_send_open_req(struct qcom_glink *glink,
>  				    struct glink_channel *channel)
>  {
>  	DEFINE_RAW_FLEX(struct glink_msg, req, data, GLINK_NAME_SIZE);
> -	int name_len = strlen(channel->name) + 1;
> -	int req_len = ALIGN(sizeof(*req) + name_len, 8);
> +	int name_len, req_len;
>  	int ret;
>  	unsigned long flags;
>  
> @@ -498,14 +497,20 @@ static int qcom_glink_send_open_req(struct qcom_glink *glink,
>  
>  	channel->lcid = ret;
>  
> +	name_len = strscpy_pad(req->data, channel->name, GLINK_NAME_SIZE);
> +	if (name_len < 0)
> +		name_len = GLINK_NAME_SIZE;
> +	else
> +		name_len++;

Should we perhaps do something along the lines of:

WARN_ON(strlen(name) > GLINK_NAME_SIZE)

to prevent silent clipping?

Konrad

Re: [PATCH next] drivers/rpmsg: Fix copy of channel->name into open request

[David Laight]

On Thu, 18 Jun 2026 15:24:17 +0200
Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> wrote:

> On 6/8/26 11:55 AM, david.laight.linux@gmail.com wrote:
> > From: David Laight <david.laight.linux@gmail.com>
> > 
> > Nothing obvious ensures that the name is less than GLINK_CMD_OPEN (32)  
> 						     ^ GLINK_NAME_SIZE

I was writing a lot of commit messages, most with -m 'text'.

> [...]
> 
> > @@ -481,8 +481,7 @@ static int qcom_glink_send_open_req(struct qcom_glink *glink,
> >  				    struct glink_channel *channel)
> >  {
> >  	DEFINE_RAW_FLEX(struct glink_msg, req, data, GLINK_NAME_SIZE);
> > -	int name_len = strlen(channel->name) + 1;
> > -	int req_len = ALIGN(sizeof(*req) + name_len, 8);
> > +	int name_len, req_len;
> >  	int ret;
> >  	unsigned long flags;
> >  
> > @@ -498,14 +497,20 @@ static int qcom_glink_send_open_req(struct qcom_glink *glink,
> >  
> >  	channel->lcid = ret;
> >  
> > +	name_len = strscpy_pad(req->data, channel->name, GLINK_NAME_SIZE);
> > +	if (name_len < 0)
> > +		name_len = GLINK_NAME_SIZE;
> > +	else
> > +		name_len++;  
> 
> Should we perhaps do something along the lines of:
> 
> WARN_ON(strlen(name) > GLINK_NAME_SIZE)
> 
> to prevent silent clipping?

strscpy() tells you whether the copy got truncated.
No point calling strlen() again.
But I'm not really sure it is worth it.
Any length check of user-supplied names should be much earlier,
this is just ensuring this code doesn't overwrite its own stack.
(and ensuring stale stack doesn't get sent as padding).

	David

> 
> Konrad