| Message ID | 20230927-strncpy-drivers-mmc-host-vub300-c-v1-1-77426f62eef4@google.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 4b9b94766534ea23bffc8606b73fb18501b49a67 |
| Headers | show |
| Series | mmc: vub300: replace deprecated strncpy with strscpy | expand |
On Wed, 27 Sept 2023 at 08:41, Justin Stitt <justinstitt@google.com> 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 expect `vub300->vub_name` to be NUL-terminated based on its uses with > format strings: > | dev_info(&vub300->udev->dev, "using %s for SDIO offload processing\n", > | vub300->vub_name); > > NUL-padding is not needed. We can see cleaning out vub_name simply > consists of: > | vub300->vub_name[0] = 0; > > Considering the above, for all 11 cases a suitable replacement is > `strscpy` [2] due to the fact that it guarantees NUL-termination on the > destination buffer without unnecessarily NUL-padding. > > To be clear, there is no existing bug in the current implementation as > the string literals are all small enough as to not cause a buffer > overread. Nonetheless, this gets us 11 steps closer to removing strncpy > uses. > > 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: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt <justinstitt@google.com> Applied for next, thanks! Kind regards Uffe > --- > Note: build-tested only. > --- > drivers/mmc/host/vub300.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/drivers/mmc/host/vub300.c b/drivers/mmc/host/vub300.c > index 9ec593d52f0f..de3f443f5fdc 100644 > --- a/drivers/mmc/host/vub300.c > +++ b/drivers/mmc/host/vub300.c > @@ -512,7 +512,7 @@ static void new_system_port_status(struct vub300_mmc_host *vub300) > vub300->card_present = 1; > vub300->bus_width = 0; > if (disable_offload_processing) > - strncpy(vub300->vub_name, "EMPTY Processing Disabled", > + strscpy(vub300->vub_name, "EMPTY Processing Disabled", > sizeof(vub300->vub_name)); > else > vub300->vub_name[0] = 0; > @@ -1216,7 +1216,7 @@ static void __download_offload_pseudocode(struct vub300_mmc_host *vub300, > dev_err(&vub300->udev->dev, > "corrupt offload pseudocode in firmware %s\n", > vub300->vub_name); > - strncpy(vub300->vub_name, "corrupt offload pseudocode", > + strscpy(vub300->vub_name, "corrupt offload pseudocode", > sizeof(vub300->vub_name)); > return; > } > @@ -1250,7 +1250,7 @@ static void __download_offload_pseudocode(struct vub300_mmc_host *vub300, > "not enough memory for xfer buffer to send" > " INTERRUPT_PSEUDOCODE for %s %s\n", fw->data, > vub300->vub_name); > - strncpy(vub300->vub_name, > + strscpy(vub300->vub_name, > "SDIO interrupt pseudocode download failed", > sizeof(vub300->vub_name)); > return; > @@ -1259,7 +1259,7 @@ static void __download_offload_pseudocode(struct vub300_mmc_host *vub300, > dev_err(&vub300->udev->dev, > "corrupt interrupt pseudocode in firmware %s %s\n", > fw->data, vub300->vub_name); > - strncpy(vub300->vub_name, "corrupt interrupt pseudocode", > + strscpy(vub300->vub_name, "corrupt interrupt pseudocode", > sizeof(vub300->vub_name)); > return; > } > @@ -1293,7 +1293,7 @@ static void __download_offload_pseudocode(struct vub300_mmc_host *vub300, > "not enough memory for xfer buffer to send" > " TRANSFER_PSEUDOCODE for %s %s\n", fw->data, > vub300->vub_name); > - strncpy(vub300->vub_name, > + strscpy(vub300->vub_name, > "SDIO transfer pseudocode download failed", > sizeof(vub300->vub_name)); > return; > @@ -1302,7 +1302,7 @@ static void __download_offload_pseudocode(struct vub300_mmc_host *vub300, > dev_err(&vub300->udev->dev, > "corrupt transfer pseudocode in firmware %s %s\n", > fw->data, vub300->vub_name); > - strncpy(vub300->vub_name, "corrupt transfer pseudocode", > + strscpy(vub300->vub_name, "corrupt transfer pseudocode", > sizeof(vub300->vub_name)); > return; > } > @@ -1336,13 +1336,13 @@ static void __download_offload_pseudocode(struct vub300_mmc_host *vub300, > dev_err(&vub300->udev->dev, > "corrupt dynamic registers in firmware %s\n", > vub300->vub_name); > - strncpy(vub300->vub_name, "corrupt dynamic registers", > + strscpy(vub300->vub_name, "corrupt dynamic registers", > sizeof(vub300->vub_name)); > return; > } > > copy_error_message: > - strncpy(vub300->vub_name, "SDIO pseudocode download failed", > + strscpy(vub300->vub_name, "SDIO pseudocode download failed", > sizeof(vub300->vub_name)); > } > > @@ -1370,11 +1370,11 @@ static void download_offload_pseudocode(struct vub300_mmc_host *vub300) > vub300->vub_name); > retval = request_firmware(&fw, vub300->vub_name, &card->dev); > if (retval < 0) { > - strncpy(vub300->vub_name, "vub_default.bin", > + strscpy(vub300->vub_name, "vub_default.bin", > sizeof(vub300->vub_name)); > retval = request_firmware(&fw, vub300->vub_name, &card->dev); > if (retval < 0) { > - strncpy(vub300->vub_name, > + strscpy(vub300->vub_name, > "no SDIO offload firmware found", > sizeof(vub300->vub_name)); > } else { > @@ -1758,7 +1758,7 @@ static void vub300_cmndwork_thread(struct work_struct *work) > * has been already downloaded to the VUB300 chip > */ > } else if (0 == vub300->mmc->card->sdio_funcs) { > - strncpy(vub300->vub_name, "SD memory device", > + strscpy(vub300->vub_name, "SD memory device", > sizeof(vub300->vub_name)); > } else { > download_offload_pseudocode(vub300); > > --- > base-commit: 6465e260f48790807eef06b583b38ca9789b6072 > change-id: 20230927-strncpy-drivers-mmc-host-vub300-c-b7b39f82e584 > > Best regards, > -- > Justin Stitt <justinstitt@google.com> >
diff --git a/drivers/mmc/host/vub300.c b/drivers/mmc/host/vub300.c index 9ec593d52f0f..de3f443f5fdc 100644 --- a/drivers/mmc/host/vub300.c +++ b/drivers/mmc/host/vub300.c @@ -512,7 +512,7 @@ static void new_system_port_status(struct vub300_mmc_host *vub300) vub300->card_present = 1; vub300->bus_width = 0; if (disable_offload_processing) - strncpy(vub300->vub_name, "EMPTY Processing Disabled", + strscpy(vub300->vub_name, "EMPTY Processing Disabled", sizeof(vub300->vub_name)); else vub300->vub_name[0] = 0; @@ -1216,7 +1216,7 @@ static void __download_offload_pseudocode(struct vub300_mmc_host *vub300, dev_err(&vub300->udev->dev, "corrupt offload pseudocode in firmware %s\n", vub300->vub_name); - strncpy(vub300->vub_name, "corrupt offload pseudocode", + strscpy(vub300->vub_name, "corrupt offload pseudocode", sizeof(vub300->vub_name)); return; } @@ -1250,7 +1250,7 @@ static void __download_offload_pseudocode(struct vub300_mmc_host *vub300, "not enough memory for xfer buffer to send" " INTERRUPT_PSEUDOCODE for %s %s\n", fw->data, vub300->vub_name); - strncpy(vub300->vub_name, + strscpy(vub300->vub_name, "SDIO interrupt pseudocode download failed", sizeof(vub300->vub_name)); return; @@ -1259,7 +1259,7 @@ static void __download_offload_pseudocode(struct vub300_mmc_host *vub300, dev_err(&vub300->udev->dev, "corrupt interrupt pseudocode in firmware %s %s\n", fw->data, vub300->vub_name); - strncpy(vub300->vub_name, "corrupt interrupt pseudocode", + strscpy(vub300->vub_name, "corrupt interrupt pseudocode", sizeof(vub300->vub_name)); return; } @@ -1293,7 +1293,7 @@ static void __download_offload_pseudocode(struct vub300_mmc_host *vub300, "not enough memory for xfer buffer to send" " TRANSFER_PSEUDOCODE for %s %s\n", fw->data, vub300->vub_name); - strncpy(vub300->vub_name, + strscpy(vub300->vub_name, "SDIO transfer pseudocode download failed", sizeof(vub300->vub_name)); return; @@ -1302,7 +1302,7 @@ static void __download_offload_pseudocode(struct vub300_mmc_host *vub300, dev_err(&vub300->udev->dev, "corrupt transfer pseudocode in firmware %s %s\n", fw->data, vub300->vub_name); - strncpy(vub300->vub_name, "corrupt transfer pseudocode", + strscpy(vub300->vub_name, "corrupt transfer pseudocode", sizeof(vub300->vub_name)); return; } @@ -1336,13 +1336,13 @@ static void __download_offload_pseudocode(struct vub300_mmc_host *vub300, dev_err(&vub300->udev->dev, "corrupt dynamic registers in firmware %s\n", vub300->vub_name); - strncpy(vub300->vub_name, "corrupt dynamic registers", + strscpy(vub300->vub_name, "corrupt dynamic registers", sizeof(vub300->vub_name)); return; } copy_error_message: - strncpy(vub300->vub_name, "SDIO pseudocode download failed", + strscpy(vub300->vub_name, "SDIO pseudocode download failed", sizeof(vub300->vub_name)); } @@ -1370,11 +1370,11 @@ static void download_offload_pseudocode(struct vub300_mmc_host *vub300) vub300->vub_name); retval = request_firmware(&fw, vub300->vub_name, &card->dev); if (retval < 0) { - strncpy(vub300->vub_name, "vub_default.bin", + strscpy(vub300->vub_name, "vub_default.bin", sizeof(vub300->vub_name)); retval = request_firmware(&fw, vub300->vub_name, &card->dev); if (retval < 0) { - strncpy(vub300->vub_name, + strscpy(vub300->vub_name, "no SDIO offload firmware found", sizeof(vub300->vub_name)); } else { @@ -1758,7 +1758,7 @@ static void vub300_cmndwork_thread(struct work_struct *work) * has been already downloaded to the VUB300 chip */ } else if (0 == vub300->mmc->card->sdio_funcs) { - strncpy(vub300->vub_name, "SD memory device", + strscpy(vub300->vub_name, "SD memory device", sizeof(vub300->vub_name)); } else { download_offload_pseudocode(vub300);
`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 expect `vub300->vub_name` to be NUL-terminated based on its uses with format strings: | dev_info(&vub300->udev->dev, "using %s for SDIO offload processing\n", | vub300->vub_name); NUL-padding is not needed. We can see cleaning out vub_name simply consists of: | vub300->vub_name[0] = 0; Considering the above, for all 11 cases a suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding. To be clear, there is no existing bug in the current implementation as the string literals are all small enough as to not cause a buffer overread. Nonetheless, this gets us 11 steps closer to removing strncpy uses. 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: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> --- Note: build-tested only. --- drivers/mmc/host/vub300.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) --- base-commit: 6465e260f48790807eef06b583b38ca9789b6072 change-id: 20230927-strncpy-drivers-mmc-host-vub300-c-b7b39f82e584 Best regards, -- Justin Stitt <justinstitt@google.com>