On Wed, Dec 13, 2023 at 03:15:15PM +, 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
>Sent: Tuesday, December 12, 2023 12:10 AM
>To: Hardevsinh Palaniya
>Cc: agr...@kernel.org ; anders...@kernel.org
>; Konrad Dybcio ;
>Mathieu Poirier ;
>linux-arm-...@vger.kernel.org ;
>linux-remotep...@vger.kernel.org ;
> linux-kernel@vger.kernel.org
>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
>
>>
>> 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_len, NULL, 0, true);
>>if (ret)
>> --
>> 2.25.1
>>
>>
>
> References
>
>1. mailto:quic_bjora...@quicinc.com
