In qcom_glink_send_open_req() remove error: strcpy() 'channel->name'
too large for 'req.name' (1010102 vs 32)
Signed-off-by: Hardevsinh Palaniya <[email protected]>
diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 82d460ff4777..2d6a592e1c72 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -479,7 +479,7 @@ static int qcom_glink_send_open_req(struct qcom_glink *glink,
req.msg.cmd = cpu_to_le16(GLINK_CMD_OPEN);
req.msg.param1 = cpu_to_le16(channel->lcid);
req.msg.param2 = cpu_to_le32(name_len);
- strcpy(req.name, channel->name);
+ strscpy_pad(req.name, channel->name, sizeof(req.name));
ret = qcom_glink_tx(glink, &req, req_len, NULL, 0, true);
if (ret)
--
2.25.1
On Mon, Dec 11, 2023 at 09:32:20PM +0530, Hardevsinh Palaniya wrote:
> In qcom_glink_send_open_req() remove error: strcpy() 'channel->name'
> too large for 'req.name' (1010102 vs 32)
>
As far as I can tell, channel->name comes from the struct
rpmsg_channel_info->name, which is a 32-byte array, and all code paths I
can find either uses strscpy() or explicitly NUL-terminates this string.
I'm curious to know which path took us here.
> Signed-off-by: Hardevsinh Palaniya <[email protected]>
>
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 82d460ff4777..2d6a592e1c72 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -479,7 +479,7 @@ static int qcom_glink_send_open_req(struct qcom_glink *glink,
> req.msg.cmd = cpu_to_le16(GLINK_CMD_OPEN);
> req.msg.param1 = cpu_to_le16(channel->lcid);
> req.msg.param2 = cpu_to_le32(name_len);
> - strcpy(req.name, channel->name);
> + strscpy_pad(req.name, channel->name, sizeof(req.name));
I think this patch is incomplete. While it makes sure we don't overwrite
the stack. name_len is strlen(channel->name) + 1 and the amount of data
sent out is based on name_len.
As such, if you can get here with a @name of arbitrary length, then you
can control how much of the stack we're going to now leak to the
recipient.
Regards,
Bjorn
>
> ret = qcom_glink_tx(glink, &req, req_len, NULL, 0, true);
> if (ret)
> --
> 2.25.1
>
>
On Wed, Dec 13, 2023 at 03:15:15PM +0000, Hardevsinh Palaniya wrote:
> Hello [1]@Bjorn Andersson,
Please use appropriate mail list etiquette, and avoid HTML and
top-posting in your responses.
>
> "strscpy_pad" itself takes care of null-terminated strings. So, there
> will be no leak.
Your strscpy_pad() will NUL-terminate and zero-pad up to 32 bytes.
Following this the next line will write strlen(name) + sizeof(hdr) bytes
to the FIFO.
So if strlen(name) >= 32 you will read beyond the end of the zero-padded
string.
Regards,
Bjorn
> __________________________________________________________________
>
> From: Bjorn Andersson <[email protected]>
> Sent: Tuesday, December 12, 2023 12:10 AM
> To: Hardevsinh Palaniya <[email protected]>
> Cc: [email protected] <[email protected]>; [email protected]
> <[email protected]>; Konrad Dybcio <[email protected]>;
> Mathieu Poirier <[email protected]>;
> [email protected] <[email protected]>;
> [email protected] <[email protected]>;
> [email protected] <[email protected]>
> Subject: Re: [PATCH] rpmsg: glink: Fix buffer overflow
>
> On Mon, Dec 11, 2023 at 09:32:20PM +0530, Hardevsinh Palaniya wrote:
> > In qcom_glink_send_open_req() remove error: strcpy() 'channel->name'
> > too large for 'req.name' (1010102 vs 32)
> >
> As far as I can tell, channel->name comes from the struct
> rpmsg_channel_info->name, which is a 32-byte array, and all code paths
> I
> can find either uses strscpy() or explicitly NUL-terminates this
> string.
> I'm curious to know which path took us here.
> > Signed-off-by: Hardevsinh Palaniya
> <[email protected]>
> >
> > diff --git a/drivers/rpmsg/qcom_glink_native.c
> b/drivers/rpmsg/qcom_glink_native.c
> > index 82d460ff4777..2d6a592e1c72 100644
> > --- a/drivers/rpmsg/qcom_glink_native.c
> > +++ b/drivers/rpmsg/qcom_glink_native.c
> > @@ -479,7 +479,7 @@ static int qcom_glink_send_open_req(struct
> qcom_glink *glink,
> > req.msg.cmd = cpu_to_le16(GLINK_CMD_OPEN);
> > req.msg.param1 = cpu_to_le16(channel->lcid);
> > req.msg.param2 = cpu_to_le32(name_len);
> > - strcpy(req.name, channel->name);
> > + strscpy_pad(req.name, channel->name, sizeof(req.name));
> I think this patch is incomplete. While it makes sure we don't
> overwrite
> the stack. name_len is strlen(channel->name) + 1 and the amount of data
> sent out is based on name_len.
> As such, if you can get here with a @name of arbitrary length, then you
> can control how much of the stack we're going to now leak to the
> recipient.
> Regards,
> Bjorn
> >
> > ret = qcom_glink_tx(glink, &req, req_len, NULL, 0, true);
> > if (ret)
> > --
> > 2.25.1
> >
> >
>
> References
>
> 1. mailto:[email protected]