strncpy() is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.
We don't need the NUL-padding behavior that strncpy() provides as vscsi
is NUL-allocated in ibmvscsis_probe() which proceeds to call
ibmvscsis_adapter_info():
| vscsi = kzalloc(sizeof(*vscsi), GFP_KERNEL);
ibmvscsis_probe() -> ibmvscsis_handle_crq() -> ibmvscsis_parse_command()
-> ibmvscsis_mad() -> ibmvscsis_process_mad() -> ibmvscsis_adapter_info()
Following the same idea, `partition_name` is defiend as:
| static char partition_name[PARTITION_NAMELEN] = "UNKNOWN";
... which is NUL-padded already, meaning strscpy() is the best option.
Considering the above, a suitable replacement is `strscpy` [2] due to
the fact that it guarantees NUL-termination on the destination buffer
without unnecessarily NUL-padding.
However, for cap->name let's use strscpy_pad as cap is allocated via
dma_alloc_coherent():
| cap = dma_alloc_coherent(&vscsi->dma_dev->dev, olen, &token,
| GFP_ATOMIC);
Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
Note: build-tested only.
Found with: $ rg "strncpy\("
---
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index 385f812b8793..cd223ef696e5 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -1551,17 +1551,17 @@ static long ibmvscsis_adapter_info(struct scsi_info *vscsi,
if (vscsi->client_data.partition_number == 0)
vscsi->client_data.partition_number =
be32_to_cpu(info->partition_number);
- strncpy(vscsi->client_data.srp_version, info->srp_version,
+ strscpy(vscsi->client_data.srp_version, info->srp_version,
sizeof(vscsi->client_data.srp_version));
- strncpy(vscsi->client_data.partition_name, info->partition_name,
+ strscpy(vscsi->client_data.partition_name, info->partition_name,
sizeof(vscsi->client_data.partition_name));
vscsi->client_data.mad_version = be32_to_cpu(info->mad_version);
vscsi->client_data.os_type = be32_to_cpu(info->os_type);
/* Copy our info */
- strncpy(info->srp_version, SRP_VERSION,
+ strscpy(info->srp_version, SRP_VERSION,
sizeof(info->srp_version));
- strncpy(info->partition_name, vscsi->dds.partition_name,
+ strscpy(info->partition_name, vscsi->dds.partition_name,
sizeof(info->partition_name));
info->partition_number = cpu_to_be32(vscsi->dds.partition_num);
info->mad_version = cpu_to_be32(MAD_VERSION_1);
@@ -1645,8 +1645,8 @@ static int ibmvscsis_cap_mad(struct scsi_info *vscsi, struct iu_entry *iue)
be64_to_cpu(mad->buffer),
vscsi->dds.window[LOCAL].liobn, token);
if (rc == H_SUCCESS) {
- strncpy(cap->name, dev_name(&vscsi->dma_dev->dev),
- SRP_MAX_LOC_LEN);
+ strscpy_pad(cap->name, dev_name(&vscsi->dma_dev->dev),
+ sizeof(cap->name));
len = olen - min_len;
status = VIOSRP_MAD_SUCCESS;
@@ -3650,7 +3650,7 @@ static int ibmvscsis_get_system_info(void)
name = of_get_property(rootdn, "ibm,partition-name", NULL);
if (name)
- strncpy(partition_name, name, sizeof(partition_name));
+ strscpy(partition_name, name, sizeof(partition_name));
num = of_get_property(rootdn, "ibm,partition-no", NULL);
if (num)
---
base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa
change-id: 20231030-strncpy-drivers-scsi-ibmvscsi_tgt-ibmvscsi_tgt-c-8a9bd0e54666
Best regards,
--
Justin Stitt <[email protected]>
On Mon, Oct 30, 2023 at 09:43:20PM +0000, Justin Stitt wrote:
> strncpy() is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous string
> interfaces.
>
> We don't need the NUL-padding behavior that strncpy() provides as vscsi
> is NUL-allocated in ibmvscsis_probe() which proceeds to call
> ibmvscsis_adapter_info():
> | vscsi = kzalloc(sizeof(*vscsi), GFP_KERNEL);
>
> ibmvscsis_probe() -> ibmvscsis_handle_crq() -> ibmvscsis_parse_command()
> -> ibmvscsis_mad() -> ibmvscsis_process_mad() -> ibmvscsis_adapter_info()
>
> Following the same idea, `partition_name` is defiend as:
> | static char partition_name[PARTITION_NAMELEN] = "UNKNOWN";
>
> ... which is NUL-padded already, meaning strscpy() is the best option.
>
> Considering the above, a suitable replacement is `strscpy` [2] due to
> the fact that it guarantees NUL-termination on the destination buffer
> without unnecessarily NUL-padding.
My only worry here is that I don't see if %NUL termination is _required_
for these variables. (i.e. do we run the risk of truncating these by 1
byte if they're right at the limit?) Are they __nonstring?
I *think* they're %NUL terminated since they follow the same sizing as
the global "partition_name", but I'm not sure.
Can any of the SCSI authors comment on this?
>
> However, for cap->name let's use strscpy_pad as cap is allocated via
> dma_alloc_coherent():
> | cap = dma_alloc_coherent(&vscsi->dma_dev->dev, olen, &token,
> | GFP_ATOMIC);
This is also true for the "info" allocation (it comes out of DMA).
>
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
> ---
> Note: build-tested only.
>
> Found with: $ rg "strncpy\("
> ---
> drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> index 385f812b8793..cd223ef696e5 100644
> --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> @@ -1551,17 +1551,17 @@ static long ibmvscsis_adapter_info(struct scsi_info *vscsi,
> if (vscsi->client_data.partition_number == 0)
> vscsi->client_data.partition_number =
> be32_to_cpu(info->partition_number);
> - strncpy(vscsi->client_data.srp_version, info->srp_version,
> + strscpy(vscsi->client_data.srp_version, info->srp_version,
> sizeof(vscsi->client_data.srp_version));
> - strncpy(vscsi->client_data.partition_name, info->partition_name,
> + strscpy(vscsi->client_data.partition_name, info->partition_name,
> sizeof(vscsi->client_data.partition_name));
> vscsi->client_data.mad_version = be32_to_cpu(info->mad_version);
> vscsi->client_data.os_type = be32_to_cpu(info->os_type);
>
> /* Copy our info */
> - strncpy(info->srp_version, SRP_VERSION,
> + strscpy(info->srp_version, SRP_VERSION,
> sizeof(info->srp_version));
> - strncpy(info->partition_name, vscsi->dds.partition_name,
> + strscpy(info->partition_name, vscsi->dds.partition_name,
> sizeof(info->partition_name));
Since "info" is from DMA, let's use the _pad variant here just to be
safe.
> info->partition_number = cpu_to_be32(vscsi->dds.partition_num);
> info->mad_version = cpu_to_be32(MAD_VERSION_1);
> @@ -1645,8 +1645,8 @@ static int ibmvscsis_cap_mad(struct scsi_info *vscsi, struct iu_entry *iue)
> be64_to_cpu(mad->buffer),
> vscsi->dds.window[LOCAL].liobn, token);
> if (rc == H_SUCCESS) {
> - strncpy(cap->name, dev_name(&vscsi->dma_dev->dev),
> - SRP_MAX_LOC_LEN);
> + strscpy_pad(cap->name, dev_name(&vscsi->dma_dev->dev),
> + sizeof(cap->name));
And this is a safe conversion to sizeof():
struct capabilities {
...
char name[SRP_MAX_LOC_LEN];
If we can convince ourselves that non of these are __nonstring types,
then I think with the "info" change above, this should be good.
-Kees
--
Kees Cook
On 11/30/23 13:25, Kees Cook wrote:
> On Mon, Oct 30, 2023 at 09:43:20PM +0000, Justin Stitt wrote:
>> strncpy() is deprecated for use on NUL-terminated destination strings
>> [1] and as such we should prefer more robust and less ambiguous string
>> interfaces.
>>
>> We don't need the NUL-padding behavior that strncpy() provides as vscsi
>> is NUL-allocated in ibmvscsis_probe() which proceeds to call
>> ibmvscsis_adapter_info():
>> | vscsi = kzalloc(sizeof(*vscsi), GFP_KERNEL);
>>
>> ibmvscsis_probe() -> ibmvscsis_handle_crq() -> ibmvscsis_parse_command()
>> -> ibmvscsis_mad() -> ibmvscsis_process_mad() -> ibmvscsis_adapter_info()
>>
>> Following the same idea, `partition_name` is defiend as:
>> | static char partition_name[PARTITION_NAMELEN] = "UNKNOWN";
>>
>> ... which is NUL-padded already, meaning strscpy() is the best option.
>>
>> Considering the above, a suitable replacement is `strscpy` [2] due to
>> the fact that it guarantees NUL-termination on the destination buffer
>> without unnecessarily NUL-padding.
>
> My only worry here is that I don't see if %NUL termination is _required_
> for these variables. (i.e. do we run the risk of truncating these by 1
> byte if they're right at the limit?) Are they __nonstring?
>
> I *think* they're %NUL terminated since they follow the same sizing as
> the global "partition_name", but I'm not sure.
>
> Can any of the SCSI authors comment on this?
Sorry, for a delayed response. I've just taken over the maintainer role as it
had been left unaccounted for sometime.
These are meant to be handled as C strings and nul termination is expected.
-Tyrel
>
>>
>> However, for cap->name let's use strscpy_pad as cap is allocated via
>> dma_alloc_coherent():
>> | cap = dma_alloc_coherent(&vscsi->dma_dev->dev, olen, &token,
>> | GFP_ATOMIC);
>
> This is also true for the "info" allocation (it comes out of DMA).
>
>>
>> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
>> Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
>> Link: https://github.com/KSPP/linux/issues/90
>> Cc: [email protected]
>> Signed-off-by: Justin Stitt <[email protected]>
>> ---
>> Note: build-tested only.
>>
>> Found with: $ rg "strncpy\("
>> ---
>> drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 14 +++++++-------
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
>> index 385f812b8793..cd223ef696e5 100644
>> --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
>> +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
>> @@ -1551,17 +1551,17 @@ static long ibmvscsis_adapter_info(struct scsi_info *vscsi,
>> if (vscsi->client_data.partition_number == 0)
>> vscsi->client_data.partition_number =
>> be32_to_cpu(info->partition_number);
>> - strncpy(vscsi->client_data.srp_version, info->srp_version,
>> + strscpy(vscsi->client_data.srp_version, info->srp_version,
>> sizeof(vscsi->client_data.srp_version));
>> - strncpy(vscsi->client_data.partition_name, info->partition_name,
>> + strscpy(vscsi->client_data.partition_name, info->partition_name,
>> sizeof(vscsi->client_data.partition_name));
>> vscsi->client_data.mad_version = be32_to_cpu(info->mad_version);
>> vscsi->client_data.os_type = be32_to_cpu(info->os_type);
>>
>> /* Copy our info */
>> - strncpy(info->srp_version, SRP_VERSION,
>> + strscpy(info->srp_version, SRP_VERSION,
>> sizeof(info->srp_version));
>> - strncpy(info->partition_name, vscsi->dds.partition_name,
>> + strscpy(info->partition_name, vscsi->dds.partition_name,
>> sizeof(info->partition_name));
>
> Since "info" is from DMA, let's use the _pad variant here just to be
> safe.
>
>> info->partition_number = cpu_to_be32(vscsi->dds.partition_num);
>> info->mad_version = cpu_to_be32(MAD_VERSION_1);
>> @@ -1645,8 +1645,8 @@ static int ibmvscsis_cap_mad(struct scsi_info *vscsi, struct iu_entry *iue)
>> be64_to_cpu(mad->buffer),
>> vscsi->dds.window[LOCAL].liobn, token);
>> if (rc == H_SUCCESS) {
>> - strncpy(cap->name, dev_name(&vscsi->dma_dev->dev),
>> - SRP_MAX_LOC_LEN);
>> + strscpy_pad(cap->name, dev_name(&vscsi->dma_dev->dev),
>> + sizeof(cap->name));
>
> And this is a safe conversion to sizeof():
>
> struct capabilities {
> ...
> char name[SRP_MAX_LOC_LEN];
>
>
> If we can convince ourselves that non of these are __nonstring types,
> then I think with the "info" change above, this should be good.
>
> -Kees
>