[PATCH v2 1/2] soc: qcom: cmd-db: replace strncpy() with strscpy_pad()

[PATCH v2 1/2] soc: qcom: cmd-db: replace strncpy() with strscpy_pad()

[Krzysztof Kozlowski]

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));
 
 	for (i = 0; i < MAX_SLV_ID; i++) {
 		rsc_hdr = &cmd_db_header->header[i];
-- 
2.32.0

[PATCH v2 2/2] soc: qcom: correct kerneldoc

[Krzysztof Kozlowski]

Correct kerneldoc warnings like:

  drivers/soc/qcom/mdt_loader.c:126:
    warning: Function parameter or member 'fw_name' not described in 'qcom_mdt_read_metadata'

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/mdt_loader.c | 4 +++-
 drivers/soc/qcom/smp2p.c      | 3 +++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 366db493579b..3f11554df2f3 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -108,6 +108,8 @@ EXPORT_SYMBOL_GPL(qcom_mdt_get_size);
  * qcom_mdt_read_metadata() - read header and metadata from mdt or mbn
  * @fw:		firmware of mdt header or mbn
  * @data_len:	length of the read metadata blob
+ * @fw_name:	name of the firmware, for construction of segment file names
+ * @dev:	device handle to associate resources with
  *
  * The mechanism that performs the authentication of the loading firmware
  * expects an ELF header directly followed by the segment of hashes, with no
@@ -192,7 +194,7 @@ EXPORT_SYMBOL_GPL(qcom_mdt_read_metadata);
  * qcom_mdt_pas_init() - initialize PAS region for firmware loading
  * @dev:	device handle to associate resources with
  * @fw:		firmware object for the mdt file
- * @firmware:	name of the firmware, for construction of segment file names
+ * @fw_name:	name of the firmware, for construction of segment file names
  * @pas_id:	PAS identifier
  * @mem_phys:	physical address of allocated memory region
  * @ctx:	PAS metadata context, to be released by caller
diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
index 59dbf4b61e6c..d9c28a8a7cbf 100644
--- a/drivers/soc/qcom/smp2p.c
+++ b/drivers/soc/qcom/smp2p.c
@@ -119,6 +119,9 @@ struct smp2p_entry {
  * @out:	pointer to the outbound smem item
  * @smem_items:	ids of the two smem items
  * @valid_entries: already scanned inbound entries
+ * @ssr_ack_enabled: SMP2P_FEATURE_SSR_ACK feature is supported and was enabled
+ * @ssr_ack: current cached state of the local ack bit
+ * @negotiation_done: whether negotiating finished
  * @local_pid:	processor id of the inbound edge
  * @remote_pid:	processor id of the outbound edge
  * @ipc_regmap:	regmap for the outbound ipc
-- 
2.32.0

Re: [PATCH v2 1/2] soc: qcom: cmd-db: replace strncpy() with strscpy_pad()

[Krzysztof Kozlowski]

On 19/05/2022 09:33, Krzysztof Kozlowski 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>

Any comments on the set?

Best regards,
Krzysztof

Re: [PATCH v2 1/2] soc: qcom: cmd-db: replace strncpy() with strscpy_pad()

[Bjorn Andersson]

On Thu, 19 May 2022 09:33:00 +0200, Krzysztof Kozlowski 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));
> 
> [...]

Applied, thanks!

[1/2] soc: qcom: cmd-db: replace strncpy() with strscpy_pad()
      commit: ac0126a0173531d91d164e244ed1ebbee64bcd54
[2/2] soc: qcom: correct kerneldoc
      commit: d11a34a404ee5565ce8e0abe3e2b9ce6f5cc0a4b

Best regards,
-- 
Bjorn Andersson <bjorn.andersson@linaro.org>

Re: [PATCH v2 1/2] soc: qcom: cmd-db: replace strncpy() with strscpy_pad()

[Doug Anderson]

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. 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".

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.

-Doug

Re: [PATCH v2 1/2] soc: qcom: cmd-db: replace strncpy() with strscpy_pad()

[Doug Anderson]

Hi,

On Mon, Jun 27, 2022 at 4:06 PM Doug Anderson <dianders@chromium.org> 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. 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".
>
> 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.

Actually, I followed the link on the error message and found that the
right solution is probably '__nonstring' so I've posted that. If
someone wants the bikeshed a different color I can do it a different
way.

https://lore.kernel.org/r/20220627161642.1.Ie7b480cd99e2c13319220cbc108caf2bcd41286b@changeid

Re: [PATCH v2 1/2] soc: qcom: cmd-db: replace strncpy() with strscpy_pad()

[Krzysztof Kozlowski]

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