lkml.org 
[lkml]   [2022]   [Jun]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 1/2] soc: qcom: cmd-db: replace strncpy() with strscpy_pad()
From
On 28/06/2022 01:06, Doug Anderson wrote:
> Hi,
>
> On Thu, May 19, 2022 at 12:33 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> The use of strncpy() is considered deprecated for NUL-terminated
>> strings[1]. Replace strncpy() with strscpy_pad(), to keep existing
>> pad-behavior of strncpy. This fixes W=1 warning:
>>
>> drivers/soc/qcom/cmd-db.c: In function ‘cmd_db_get_header.part.0’:
>> drivers/soc/qcom/cmd-db.c:151:9: warning: ‘strncpy’ specified bound 8 equals destination size [-Wstringop-truncation]
>> 151 | strncpy(query, id, sizeof(query));
>>
>> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
>>
>> ---
>>
>> Changes since v1:
>> 1. Split series per subsystem.
>> 2. Add tag.
>> ---
>> drivers/soc/qcom/cmd-db.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c
>> index dd872017f345..c5137c25d819 100644
>> --- a/drivers/soc/qcom/cmd-db.c
>> +++ b/drivers/soc/qcom/cmd-db.c
>> @@ -148,7 +148,7 @@ static int cmd_db_get_header(const char *id, const struct entry_header **eh,
>> return ret;
>>
>> /* Pad out query string to same length as in DB */
>> - strncpy(query, id, sizeof(query));
>> + strscpy_pad(query, id, sizeof(query));
>
> Sorry to report that this breaks booting on
> sc7280-herobrine-herobrine-r1.

Sorry to break booting and thanks for the report.

> I believe that the function was
> actually _relying_ on the "unsafe" behavior of strncpy(). Specifically
> I think:
> * The source string (id) was a '\0' terminated string.
> * The destination (query) was a fixed 8-byte string and doesn't need
> to be '\0' terminated.
>
> So before your change we could actually support 8-byte strings. Now we
> can't because you'll only copy 7 bytes to the destination to leave
> room for the '\0' termination...
>
> Looking at printouts, I see, for instance at least one ID that looks
> like "lnbclka2".

Ah, crap... I did not expect that.

>
> Given the requirements of this function(), the old strncpy() is
> actually _exactly_ what we want. Is there any way to disable the
> warning? If not, I guess we could make "query" be 9 bytes bit, or
> "sizeof(ent->id) + 1" bytes big... Happy to post a patch, but it's
> basically a bikeshed for how you want it fixed (there are dozens of
> ways) and I'd rather you just tell me instead of me guessing.

If the source was not a string, I would propose memcpy(). Since it is a
string, I would prefer to have safe copy, so increased size. However I
see you sent now patches, so let me actually respond there.

Thanks for the analysis, much appreciated!


Best regards,
Krzysztof

\
 
 \ /
  Last update: 2022-06-28 11:53    [W:0.214 / U:0.448 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site