* [PATCH 5.10 0/1] rpmsg: qcom: glink: replace strncpy() with strscpy_pad() @ 2022-10-07 13:29 Andrew Chernyakov 2022-10-07 13:29 ` [PATCH 5.10 1/1] " Andrew Chernyakov 0 siblings, 1 reply; 6+ messages in thread From: Andrew Chernyakov @ 2022-10-07 13:29 UTC (permalink / raw) To: acherniakov, Greg Kroah-Hartman Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Mathieu Poirier, linux-arm-msm, linux-remoteproc, linux-kernel, Krzysztof Kozlowski, stable, lvc-project With static analisys tools we found that strncpy() is used in rpmsg. This function is not safe and can lead to buffer overflow. This patchset replaces strncpy() with strscpy_pad(). Link: https://lore.kernel.org/all/20220519073330.7187-1-krzysztof.kozlowski@linaro.org/ Found by Linux Verification Center (linuxtesting.org) with SVACE. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 5.10 1/1] rpmsg: qcom: glink: replace strncpy() with strscpy_pad() 2022-10-07 13:29 [PATCH 5.10 0/1] rpmsg: qcom: glink: replace strncpy() with strscpy_pad() Andrew Chernyakov @ 2022-10-07 13:29 ` Andrew Chernyakov 2022-10-07 16:40 ` Greg Kroah-Hartman 0 siblings, 1 reply; 6+ messages in thread From: Andrew Chernyakov @ 2022-10-07 13:29 UTC (permalink / raw) To: acherniakov, Greg Kroah-Hartman Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Mathieu Poirier, linux-arm-msm, linux-remoteproc, linux-kernel, Krzysztof Kozlowski, stable, lvc-project, Stephen Boyd, Bjorn Andersson From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> commit 766279a8f85df32345dbda03b102ca1ee3d5ddea upstream. The use of strncpy() is considered deprecated for NUL-terminated strings[1]. Replace strncpy() with strscpy_pad(), to keep existing pad-behavior of strncpy, similarly to commit 08de420a8014 ("rpmsg: glink: Replace strncpy() with strscpy_pad()"). This fixes W=1 warning: In function ‘qcom_glink_rx_close’, inlined from ‘qcom_glink_work’ at ../drivers/rpmsg/qcom_glink_native.c:1638:4: drivers/rpmsg/qcom_glink_native.c:1549:17: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation] 1549 | strncpy(chinfo.name, channel->name, sizeof(chinfo.name)); [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> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> Link: https://lore.kernel.org/r/20220519073330.7187-1-krzysztof.kozlowski@linaro.org Signed-off-by: Andrew Chernyakov <acherniakov@astralinux.ru> --- drivers/rpmsg/qcom_glink_native.c | 2 +- drivers/rpmsg/qcom_smd.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c index 4840886532ff..7cbed0310c09 100644 --- a/drivers/rpmsg/qcom_glink_native.c +++ b/drivers/rpmsg/qcom_glink_native.c @@ -1472,7 +1472,7 @@ static void qcom_glink_rx_close(struct qcom_glink *glink, unsigned int rcid) cancel_work_sync(&channel->intent_work); if (channel->rpdev) { - strncpy(chinfo.name, channel->name, sizeof(chinfo.name)); + strscpy_pad(chinfo.name, channel->name, sizeof(chinfo.name)); chinfo.src = RPMSG_ADDR_ANY; chinfo.dst = RPMSG_ADDR_ANY; diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c index 0b1e853d8c91..b5167ef93abf 100644 --- a/drivers/rpmsg/qcom_smd.c +++ b/drivers/rpmsg/qcom_smd.c @@ -1073,7 +1073,7 @@ static int qcom_smd_create_device(struct qcom_smd_channel *channel) /* Assign public information to the rpmsg_device */ rpdev = &qsdev->rpdev; - strncpy(rpdev->id.name, channel->name, RPMSG_NAME_SIZE); + strscpy_pad(rpdev->id.name, channel->name, RPMSG_NAME_SIZE); rpdev->src = RPMSG_ADDR_ANY; rpdev->dst = RPMSG_ADDR_ANY; @@ -1304,7 +1304,7 @@ static void qcom_channel_state_worker(struct work_struct *work) spin_unlock_irqrestore(&edge->channels_lock, flags); - strncpy(chinfo.name, channel->name, sizeof(chinfo.name)); + strscpy_pad(chinfo.name, channel->name, sizeof(chinfo.name)); chinfo.src = RPMSG_ADDR_ANY; chinfo.dst = RPMSG_ADDR_ANY; rpmsg_unregister_device(&edge->dev, &chinfo); -- 2.35.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 5.10 1/1] rpmsg: qcom: glink: replace strncpy() with strscpy_pad() 2022-10-07 13:29 ` [PATCH 5.10 1/1] " Andrew Chernyakov @ 2022-10-07 16:40 ` Greg Kroah-Hartman 2022-10-08 21:11 ` David Laight 0 siblings, 1 reply; 6+ messages in thread From: Greg Kroah-Hartman @ 2022-10-07 16:40 UTC (permalink / raw) To: Andrew Chernyakov Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Mathieu Poirier, linux-arm-msm, linux-remoteproc, linux-kernel, Krzysztof Kozlowski, stable, lvc-project, Stephen Boyd, Bjorn Andersson On Fri, Oct 07, 2022 at 04:29:31PM +0300, Andrew Chernyakov wrote: > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > commit 766279a8f85df32345dbda03b102ca1ee3d5ddea upstream. > > The use of strncpy() is considered deprecated for NUL-terminated > strings[1]. Replace strncpy() with strscpy_pad(), to keep existing > pad-behavior of strncpy, similarly to commit 08de420a8014 ("rpmsg: > glink: Replace strncpy() with strscpy_pad()"). This fixes W=1 warning: > > In function ‘qcom_glink_rx_close’, > inlined from ‘qcom_glink_work’ at ../drivers/rpmsg/qcom_glink_native.c:1638:4: > drivers/rpmsg/qcom_glink_native.c:1549:17: warning: ‘strncpy’ specified bound 32 equals destination size [-Wstringop-truncation] > 1549 | strncpy(chinfo.name, channel->name, sizeof(chinfo.name)); > > [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> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Link: https://lore.kernel.org/r/20220519073330.7187-1-krzysztof.kozlowski@linaro.org > Signed-off-by: Andrew Chernyakov <acherniakov@astralinux.ru> > --- > drivers/rpmsg/qcom_glink_native.c | 2 +- > drivers/rpmsg/qcom_smd.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) Why just this specific kernel branch? We can't add patches to an older tree and have someone upgrade to a newer one and hit the same issue. So please provide backports for all active versions. In this case that would be 5.15.y and 5.19.y. thanks, greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 5.10 1/1] rpmsg: qcom: glink: replace strncpy() with strscpy_pad() 2022-10-07 16:40 ` Greg Kroah-Hartman @ 2022-10-08 21:11 ` David Laight 2022-10-09 15:23 ` Krzysztof Kozlowski 0 siblings, 1 reply; 6+ messages in thread From: David Laight @ 2022-10-08 21:11 UTC (permalink / raw) To: 'Greg Kroah-Hartman', Andrew Chernyakov Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Mathieu Poirier, linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org, Krzysztof Kozlowski, stable@vger.kernel.org, lvc-project@linuxtesting.org, Stephen Boyd, Bjorn Andersson From: Greg Kroah-Hartman > Sent: 07 October 2022 17:40 > > On Fri, Oct 07, 2022 at 04:29:31PM +0300, Andrew Chernyakov wrote: > > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > > > commit 766279a8f85df32345dbda03b102ca1ee3d5ddea upstream. > > > > The use of strncpy() is considered deprecated for NUL-terminated > > strings[1]. Replace strncpy() with strscpy_pad(), to keep existing > > pad-behavior of strncpy, similarly to commit 08de420a8014 ("rpmsg: > > glink: Replace strncpy() with strscpy_pad()"). This fixes W=1 warning: > > > > In function ‘qcom_glink_rx_close’, > > inlined from ‘qcom_glink_work’ at ../drivers/rpmsg/qcom_glink_native.c:1638:4: > > drivers/rpmsg/qcom_glink_native.c:1549:17: warning: ‘strncpy’ specified bound 32 equals > destination size [-Wstringop-truncation] > > 1549 | strncpy(chinfo.name, channel->name, sizeof(chinfo.name)); > > > > [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> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > Link: https://lore.kernel.org/r/20220519073330.7187-1-krzysztof.kozlowski@linaro.org > > Signed-off-by: Andrew Chernyakov <acherniakov@astralinux.ru> > > --- > > drivers/rpmsg/qcom_glink_native.c | 2 +- > > drivers/rpmsg/qcom_smd.c | 4 ++-- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > Why just this specific kernel branch? We can't add patches to an older > tree and have someone upgrade to a newer one and hit the same issue. > > So please provide backports for all active versions. In this case that > would be 5.15.y and 5.19.y. If it is only fixing a compile warning is it even stable material? The generic commit message doesn't say whether the old code was actually right or wrong. At least one of these 'replace strncpy()' changes was definitely broken (the copy needed to be equivalent to memcpy()). So applying ANY of them to stable unless they actually fix a real bug seems dubious. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 5.10 1/1] rpmsg: qcom: glink: replace strncpy() with strscpy_pad() 2022-10-08 21:11 ` David Laight @ 2022-10-09 15:23 ` Krzysztof Kozlowski 2022-10-09 18:17 ` 'Greg Kroah-Hartman' 0 siblings, 1 reply; 6+ messages in thread From: Krzysztof Kozlowski @ 2022-10-09 15:23 UTC (permalink / raw) To: David Laight, 'Greg Kroah-Hartman', Andrew Chernyakov Cc: Andy Gross, Bjorn Andersson, Konrad Dybcio, Mathieu Poirier, linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org, lvc-project@linuxtesting.org, Stephen Boyd, Bjorn Andersson On 08/10/2022 23:11, David Laight wrote: >>> --- >>> drivers/rpmsg/qcom_glink_native.c | 2 +- >>> drivers/rpmsg/qcom_smd.c | 4 ++-- >>> 2 files changed, 3 insertions(+), 3 deletions(-) >> >> Why just this specific kernel branch? We can't add patches to an older >> tree and have someone upgrade to a newer one and hit the same issue. >> >> So please provide backports for all active versions. In this case that >> would be 5.15.y and 5.19.y. > > If it is only fixing a compile warning is it even stable material? > The generic commit message doesn't say whether the old code was > actually right or wrong. > > At least one of these 'replace strncpy()' changes was definitely > broken (the copy needed to be equivalent to memcpy()). > > So applying ANY of them to stable unless they actually fix > a real bug seems dubious. Except the warning from GCC, there was no bug to fix. The warning is about discouraged and risky practice, but no actual real risk was identified, so for me it matches stable rules poorly. It's basically backporting to silence automated code checkers... Best regards, Krzysztof ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 5.10 1/1] rpmsg: qcom: glink: replace strncpy() with strscpy_pad() 2022-10-09 15:23 ` Krzysztof Kozlowski @ 2022-10-09 18:17 ` 'Greg Kroah-Hartman' 0 siblings, 0 replies; 6+ messages in thread From: 'Greg Kroah-Hartman' @ 2022-10-09 18:17 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: David Laight, Andrew Chernyakov, Andy Gross, Bjorn Andersson, Konrad Dybcio, Mathieu Poirier, linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org, lvc-project@linuxtesting.org, Stephen Boyd, Bjorn Andersson On Sun, Oct 09, 2022 at 05:23:06PM +0200, Krzysztof Kozlowski wrote: > On 08/10/2022 23:11, David Laight wrote: > >>> --- > >>> drivers/rpmsg/qcom_glink_native.c | 2 +- > >>> drivers/rpmsg/qcom_smd.c | 4 ++-- > >>> 2 files changed, 3 insertions(+), 3 deletions(-) > >> > >> Why just this specific kernel branch? We can't add patches to an older > >> tree and have someone upgrade to a newer one and hit the same issue. > >> > >> So please provide backports for all active versions. In this case that > >> would be 5.15.y and 5.19.y. > > > > If it is only fixing a compile warning is it even stable material? > > The generic commit message doesn't say whether the old code was > > actually right or wrong. > > > > At least one of these 'replace strncpy()' changes was definitely > > broken (the copy needed to be equivalent to memcpy()). > > > > So applying ANY of them to stable unless they actually fix > > a real bug seems dubious. > > Except the warning from GCC, there was no bug to fix. The warning is > about discouraged and risky practice, but no actual real risk was > identified, so for me it matches stable rules poorly. > > It's basically backporting to silence automated code checkers... Are you sure? Look at the code path here, there might be a way to overflow the string, from the virtio interface, but I might be wrong... Anyway, I need all the backports before I can take this one, sorry. greg k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-10-09 18:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-10-07 13:29 [PATCH 5.10 0/1] rpmsg: qcom: glink: replace strncpy() with strscpy_pad() Andrew Chernyakov 2022-10-07 13:29 ` [PATCH 5.10 1/1] " Andrew Chernyakov 2022-10-07 16:40 ` Greg Kroah-Hartman 2022-10-08 21:11 ` David Laight 2022-10-09 15:23 ` Krzysztof Kozlowski 2022-10-09 18:17 ` 'Greg Kroah-Hartman'
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox