| Message ID | 20230523021425.2406309-1-azeemshaikh38@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 8453e7924a1a9130f2b4d2c507de2cdc3892a5b5 |
| Headers | show |
| Series | soc: fsl: qe: Replace all non-returning strlcpy with strscpy | expand |
On Tue, May 23, 2023 at 02:14:25AM +0000, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > No return values were used, so direct replacement is safe. > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > [2] https://github.com/KSPP/linux/issues/89 > > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> Reviewed-by: Kees Cook <keescook@chromium.org>
On Tue, May 23, 2023 at 1:20 PM Kees Cook <keescook@chromium.org> wrote: > > On Tue, May 23, 2023 at 02:14:25AM +0000, Azeem Shaikh wrote: > > strlcpy() reads the entire source buffer first. > > This read may exceed the destination size limit. > > This is both inefficient and can lead to linear read > > overflows if a source string is not NUL-terminated [1]. > > In an effort to remove strlcpy() completely [2], replace > > strlcpy() here with strscpy(). > > No return values were used, so direct replacement is safe. > > > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > > [2] https://github.com/KSPP/linux/issues/89 > > > > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> > > Reviewed-by: Kees Cook <keescook@chromium.org> > Friendly ping on this.
> -----Original Message----- > From: Azeem Shaikh <azeemshaikh38@gmail.com> > Sent: Sunday, July 9, 2023 9:36 PM > To: Kees Cook <keescook@chromium.org> > Cc: Qiang Zhao <qiang.zhao@nxp.com>; linux-hardening@vger.kernel.org; > linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Leo Li > <leoyang.li@nxp.com>; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH] soc: fsl: qe: Replace all non-returning strlcpy with > strscpy > > On Tue, May 23, 2023 at 1:20 PM Kees Cook <keescook@chromium.org> > wrote: > > > > On Tue, May 23, 2023 at 02:14:25AM +0000, Azeem Shaikh wrote: > > > strlcpy() reads the entire source buffer first. > > > This read may exceed the destination size limit. > > > This is both inefficient and can lead to linear read overflows if a > > > source string is not NUL-terminated [1]. > > > In an effort to remove strlcpy() completely [2], replace > > > strlcpy() here with strscpy(). > > > No return values were used, so direct replacement is safe. > > > > > > [1] > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww > > > > w.kernel.org%2Fdoc%2Fhtml%2Flatest%2Fprocess%2Fdeprecated.html%23s > tr > > > > lcpy&data=05%7C01%7Cleoyang.li%40nxp.com%7C11f9df1df1b5440e4ec108 > db8 > > > > 0ee64de%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63824553360 > 3780 > > > > 889%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu > MzIiLCJB > > > > TiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=jcTy3IF37wqC1 > MWsSuF > > > %2F51Z1trQEMaow7BHkPSh3hzI%3D&reserved=0 > > > [2] > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi > > > > thub.com%2FKSPP%2Flinux%2Fissues%2F89&data=05%7C01%7Cleoyang.li% > 40nx > > > > p.com%7C11f9df1df1b5440e4ec108db80ee64de%7C686ea1d3bc2b4c6fa92cd > 99c5 > > > > c301635%7C0%7C0%7C638245533603780889%7CUnknown%7CTWFpbGZsb3d > 8eyJWIjo > > > > iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C30 > 00%7 > > > > C%7C%7C&sdata=Blr0W1oYPIw5uDu7HqlEkU7xOuAo4bQNkk%2Bt%2BAuFqc > s%3D&res > > > erved=0 > > > > > > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> > > > > Reviewed-by: Kees Cook <keescook@chromium.org> > > > > Friendly ping on this. Sorry for the late response. But I found some old discussions with the conclusion to be not converting old users. Has this been changed later on? https://lwn.net/Articles/659214/ Regards, Leo
> Sorry for the late response. But I found some old discussions with the conclusion to be not converting old users. Has this been changed later on? > https://lwn.net/Articles/659214/ > @Kees Cook what's your advice here?
On Mon, Jul 10, 2023 at 04:46:50PM +0000, Leo Li wrote: > > > > -----Original Message----- > > From: Azeem Shaikh <azeemshaikh38@gmail.com> > > Sent: Sunday, July 9, 2023 9:36 PM > > To: Kees Cook <keescook@chromium.org> > > Cc: Qiang Zhao <qiang.zhao@nxp.com>; linux-hardening@vger.kernel.org; > > linuxppc-dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; Leo Li > > <leoyang.li@nxp.com>; linux-arm-kernel@lists.infradead.org > > Subject: Re: [PATCH] soc: fsl: qe: Replace all non-returning strlcpy with > > strscpy > > > > On Tue, May 23, 2023 at 1:20 PM Kees Cook <keescook@chromium.org> > > wrote: > > > > > > On Tue, May 23, 2023 at 02:14:25AM +0000, Azeem Shaikh wrote: > > > > strlcpy() reads the entire source buffer first. > > > > This read may exceed the destination size limit. > > > > This is both inefficient and can lead to linear read overflows if a > > > > source string is not NUL-terminated [1]. > > > > In an effort to remove strlcpy() completely [2], replace > > > > strlcpy() here with strscpy(). > > > > No return values were used, so direct replacement is safe. > > > > > > > > [1] > > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww > > > > > > w.kernel.org%2Fdoc%2Fhtml%2Flatest%2Fprocess%2Fdeprecated.html%23s > > tr > > > > > > lcpy&data=05%7C01%7Cleoyang.li%40nxp.com%7C11f9df1df1b5440e4ec108 > > db8 > > > > > > 0ee64de%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63824553360 > > 3780 > > > > > > 889%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu > > MzIiLCJB > > > > > > TiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=jcTy3IF37wqC1 > > MWsSuF > > > > %2F51Z1trQEMaow7BHkPSh3hzI%3D&reserved=0 > > > > [2] > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi > > > > > > thub.com%2FKSPP%2Flinux%2Fissues%2F89&data=05%7C01%7Cleoyang.li% > > 40nx > > > > > > p.com%7C11f9df1df1b5440e4ec108db80ee64de%7C686ea1d3bc2b4c6fa92cd > > 99c5 > > > > > > c301635%7C0%7C0%7C638245533603780889%7CUnknown%7CTWFpbGZsb3d > > 8eyJWIjo > > > > > > iMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C30 > > 00%7 > > > > > > C%7C%7C&sdata=Blr0W1oYPIw5uDu7HqlEkU7xOuAo4bQNkk%2Bt%2BAuFqc > > s%3D&res > > > > erved=0 > > > > > > > > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> > > > > > > Reviewed-by: Kees Cook <keescook@chromium.org> > > > > > > > Friendly ping on this. > > Sorry for the late response. But I found some old discussions with the conclusion to be not converting old users. Has this been changed later on? > https://lwn.net/Articles/659214/ The objection was with _mass_ conversions due to the risk of regressions being introduced in a way that makes it hard to revert/bisect, etc. We've being long on the road to doing these conversions to eliminate strlcpy(), which continues to get introduced into the kernel, even though it is risky.
On Tue, 23 May 2023 02:14:25 +0000, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > No return values were used, so direct replacement is safe. > > [...] Applied, thanks! [1/1] soc: fsl: qe: Replace all non-returning strlcpy with strscpy (no commit info) Best regards,
diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c index b3c226eb5292..58746e570d14 100644 --- a/drivers/soc/fsl/qe/qe.c +++ b/drivers/soc/fsl/qe/qe.c @@ -524,7 +524,7 @@ int qe_upload_firmware(const struct qe_firmware *firmware) * saved microcode information and put in the new. */ memset(&qe_firmware_info, 0, sizeof(qe_firmware_info)); - strlcpy(qe_firmware_info.id, firmware->id, sizeof(qe_firmware_info.id)); + strscpy(qe_firmware_info.id, firmware->id, sizeof(qe_firmware_info.id)); qe_firmware_info.extended_modes = be64_to_cpu(firmware->extended_modes); memcpy(qe_firmware_info.vtraps, firmware->vtraps, sizeof(firmware->vtraps)); @@ -599,7 +599,7 @@ struct qe_firmware_info *qe_get_firmware_info(void) /* Copy the data into qe_firmware_info*/ sprop = of_get_property(fw, "id", NULL); if (sprop) - strlcpy(qe_firmware_info.id, sprop, + strscpy(qe_firmware_info.id, sprop, sizeof(qe_firmware_info.id)); of_property_read_u64(fw, "extended-modes",
strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. In an effort to remove strlcpy() completely [2], replace strlcpy() here with strscpy(). No return values were used, so direct replacement is safe. [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [2] https://github.com/KSPP/linux/issues/89 Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> --- drivers/soc/fsl/qe/qe.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)