* [PATCH] qed: fix uninit pointer read in qed_mcp_nvm_info_populate() @ 2024-12-11 13:40 Gianfranco Trad 2024-12-12 17:04 ` Simon Horman 0 siblings, 1 reply; 5+ messages in thread From: Gianfranco Trad @ 2024-12-11 13:40 UTC (permalink / raw) To: manishc, andrew+netdev, davem, edumazet, kuba, pabeni Cc: netdev, linux-kernel, skhan, Gianfranco Trad Coverity reports an uninit pointer read in qed_mcp_nvm_info_populate(). If qed_mcp_bist_nvm_get_num_images() returns -EOPNOTSUPP, this leads to jump to label out with nvm_info.image_att being uninit while assigning it to p_hwfn->nvm_info.image_att. Add check on rc against -EOPNOTSUPP to avoid such uninit pointer read. Closes: https://scan5.scan.coverity.com/#/project-view/63204/10063?selectedIssue=1636666 Signed-off-by: Gianfranco Trad <gianf.trad@gmail.com> --- Note: - Fixes: tag should be "7a0ea70da56e net/qed: allow old cards not supporting "num_images" to work" ? drivers/net/ethernet/qlogic/qed/qed_mcp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/qlogic/qed/qed_mcp.c b/drivers/net/ethernet/qlogic/qed/qed_mcp.c index b45efc272fdb..127943b39f61 100644 --- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c +++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c @@ -3387,7 +3387,7 @@ int qed_mcp_nvm_info_populate(struct qed_hwfn *p_hwfn) } out: /* Update hwfn's nvm_info */ - if (nvm_info.num_images) { + if (nvm_info.num_images && rc != -EOPNOTSUPP) { p_hwfn->nvm_info.num_images = nvm_info.num_images; kfree(p_hwfn->nvm_info.image_att); p_hwfn->nvm_info.image_att = nvm_info.image_att; -- 2.43.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] qed: fix uninit pointer read in qed_mcp_nvm_info_populate() 2024-12-11 13:40 [PATCH] qed: fix uninit pointer read in qed_mcp_nvm_info_populate() Gianfranco Trad @ 2024-12-12 17:04 ` Simon Horman 2024-12-13 12:13 ` Gianfranco Trad 0 siblings, 1 reply; 5+ messages in thread From: Simon Horman @ 2024-12-12 17:04 UTC (permalink / raw) To: Gianfranco Trad Cc: manishc, andrew+netdev, davem, edumazet, kuba, pabeni, netdev, linux-kernel, skhan On Wed, Dec 11, 2024 at 02:40:42PM +0100, Gianfranco Trad wrote: > Coverity reports an uninit pointer read in qed_mcp_nvm_info_populate(). > If qed_mcp_bist_nvm_get_num_images() returns -EOPNOTSUPP, this leads to > jump to label out with nvm_info.image_att being uninit while assigning it > to p_hwfn->nvm_info.image_att. > Add check on rc against -EOPNOTSUPP to avoid such uninit pointer read. > > Closes: https://scan5.scan.coverity.com/#/project-view/63204/10063?selectedIssue=1636666 > Signed-off-by: Gianfranco Trad <gianf.trad@gmail.com> > --- > Note: > - Fixes: tag should be "7a0ea70da56e net/qed: allow old cards not supporting "num_images" to work" ? > > drivers/net/ethernet/qlogic/qed/qed_mcp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/qlogic/qed/qed_mcp.c b/drivers/net/ethernet/qlogic/qed/qed_mcp.c > index b45efc272fdb..127943b39f61 100644 > --- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c > +++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c > @@ -3387,7 +3387,7 @@ int qed_mcp_nvm_info_populate(struct qed_hwfn *p_hwfn) > } > out: > /* Update hwfn's nvm_info */ > - if (nvm_info.num_images) { > + if (nvm_info.num_images && rc != -EOPNOTSUPP) { > p_hwfn->nvm_info.num_images = nvm_info.num_images; > kfree(p_hwfn->nvm_info.image_att); > p_hwfn->nvm_info.image_att = nvm_info.image_att; Are you sure that nvm_info.num_images can be non-zero if rc == -EOPNOTSUPP? The cited commit state: Commit 43645ce03e00 ("qed: Populate nvm image attribute shadow.") added support for populating flash image attributes, notably "num_images". However, some cards were not able to return this information. In such cases, the driver would return EINVAL, causing the driver to exit. Add check to return EOPNOTSUPP instead of EINVAL when the card is not able to return these information. The caller function already handles EOPNOTSUPP without error. So I would expect that nvm_info.num_images is 0. If not, perhaps an alternate fix is to make that so, either by setting it in qed_mcp_bist_nvm_get_num_images, or where the return value of qed_mcp_bist_nvm_get_num_images is checked (just before the goto out). And, in any case I think some testing is in order. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] qed: fix uninit pointer read in qed_mcp_nvm_info_populate() 2024-12-12 17:04 ` Simon Horman @ 2024-12-13 12:13 ` Gianfranco Trad 2024-12-13 13:31 ` Simon Horman 0 siblings, 1 reply; 5+ messages in thread From: Gianfranco Trad @ 2024-12-13 12:13 UTC (permalink / raw) To: Simon Horman Cc: manishc, andrew+netdev, davem, edumazet, kuba, pabeni, netdev, linux-kernel, skhan On 12/12/24 18:04, Simon Horman wrote: > On Wed, Dec 11, 2024 at 02:40:42PM +0100, Gianfranco Trad wrote: >> Coverity reports an uninit pointer read in qed_mcp_nvm_info_populate(). >> If qed_mcp_bist_nvm_get_num_images() returns -EOPNOTSUPP, this leads to >> jump to label out with nvm_info.image_att being uninit while assigning it >> to p_hwfn->nvm_info.image_att. >> Add check on rc against -EOPNOTSUPP to avoid such uninit pointer read. >> >> Closes: https://scan5.scan.coverity.com/#/project-view/63204/10063?selectedIssue=1636666 >> Signed-off-by: Gianfranco Trad <gianf.trad@gmail.com> >> --- >> Note: >> - Fixes: tag should be "7a0ea70da56e net/qed: allow old cards not supporting "num_images" to work" ? >> >> drivers/net/ethernet/qlogic/qed/qed_mcp.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/qlogic/qed/qed_mcp.c b/drivers/net/ethernet/qlogic/qed/qed_mcp.c >> index b45efc272fdb..127943b39f61 100644 >> --- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c >> +++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c >> @@ -3387,7 +3387,7 @@ int qed_mcp_nvm_info_populate(struct qed_hwfn *p_hwfn) >> } >> out: >> /* Update hwfn's nvm_info */ >> - if (nvm_info.num_images) { >> + if (nvm_info.num_images && rc != -EOPNOTSUPP) { >> p_hwfn->nvm_info.num_images = nvm_info.num_images; >> kfree(p_hwfn->nvm_info.image_att); >> p_hwfn->nvm_info.image_att = nvm_info.image_att; > Hi Simon, > Are you sure that nvm_info.num_images can be non-zero if rc == -EOPNOTSUPP? > In the coverity report, the static analyzer is able to take the true branch on nvm_info.num_images. I didn't physically reproduce this logical state as I don't possess the matching hardware. > The cited commit state: > > Commit 43645ce03e00 ("qed: Populate nvm image attribute shadow.") > added support for populating flash image attributes, notably > "num_images". However, some cards were not able to return this > information. In such cases, the driver would return EINVAL, causing the > driver to exit. > > Add check to return EOPNOTSUPP instead of EINVAL when the card is not > able to return these information. The caller function already handles > EOPNOTSUPP without error. > > So I would expect that nvm_info.num_images is 0. > > If not, perhaps an alternate fix is to make that so, either by setting > it in qed_mcp_bist_nvm_get_num_images, or where the return value of > qed_mcp_bist_nvm_get_num_images is checked (just before the goto out). > Makes sense, so something like this I suppose: --- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c +++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c @@ -3301,8 +3301,10 @@ int qed_mcp_bist_nvm_get_num_images(struct qed_hwfn *p_hwfn, if (rc) return rc; - if (((rsp & FW_MSG_CODE_MASK) == FW_MSG_CODE_UNSUPPORTED)) + if (((rsp & FW_MSG_CODE_MASK) == FW_MSG_CODE_UNSUPPORTED)) { + *num_images = 0; rc = -EOPNOTSUPP; + } Or the second option you stated. > And, in any case I think some testing is in order. I strongly agree. Let me know if I can help more with this. Thanks for your time, --Gian ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] qed: fix uninit pointer read in qed_mcp_nvm_info_populate() 2024-12-13 12:13 ` Gianfranco Trad @ 2024-12-13 13:31 ` Simon Horman 2024-12-15 1:11 ` Gianfranco Trad 0 siblings, 1 reply; 5+ messages in thread From: Simon Horman @ 2024-12-13 13:31 UTC (permalink / raw) To: Gianfranco Trad Cc: manishc, andrew+netdev, davem, edumazet, kuba, pabeni, netdev, linux-kernel, skhan On Fri, Dec 13, 2024 at 01:13:12PM +0100, Gianfranco Trad wrote: > On 12/12/24 18:04, Simon Horman wrote: > > On Wed, Dec 11, 2024 at 02:40:42PM +0100, Gianfranco Trad wrote: > > > Coverity reports an uninit pointer read in qed_mcp_nvm_info_populate(). > > > If qed_mcp_bist_nvm_get_num_images() returns -EOPNOTSUPP, this leads to > > > jump to label out with nvm_info.image_att being uninit while assigning it > > > to p_hwfn->nvm_info.image_att. > > > Add check on rc against -EOPNOTSUPP to avoid such uninit pointer read. > > > > > > Closes: https://scan5.scan.coverity.com/#/project-view/63204/10063?selectedIssue=1636666 > > > Signed-off-by: Gianfranco Trad <gianf.trad@gmail.com> > > > --- > > > Note: > > > - Fixes: tag should be "7a0ea70da56e net/qed: allow old cards not supporting "num_images" to work" ? > > > drivers/net/ethernet/qlogic/qed/qed_mcp.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/ethernet/qlogic/qed/qed_mcp.c b/drivers/net/ethernet/qlogic/qed/qed_mcp.c > > > index b45efc272fdb..127943b39f61 100644 > > > --- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c > > > +++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c > > > @@ -3387,7 +3387,7 @@ int qed_mcp_nvm_info_populate(struct qed_hwfn *p_hwfn) > > > } > > > out: > > > /* Update hwfn's nvm_info */ > > > - if (nvm_info.num_images) { > > > + if (nvm_info.num_images && rc != -EOPNOTSUPP) { > > > p_hwfn->nvm_info.num_images = nvm_info.num_images; > > > kfree(p_hwfn->nvm_info.image_att); > > > p_hwfn->nvm_info.image_att = nvm_info.image_att; > > > > Hi Simon, > > > Are you sure that nvm_info.num_images can be non-zero if rc == -EOPNOTSUPP? > > > > In the coverity report, the static analyzer is able to take the true branch > on nvm_info.num_images. I didn't physically reproduce this logical state as > I don't possess the matching hardware. > > > The cited commit state: > > > > Commit 43645ce03e00 ("qed: Populate nvm image attribute shadow.") > > added support for populating flash image attributes, notably > > "num_images". However, some cards were not able to return this > > information. In such cases, the driver would return EINVAL, causing the > > driver to exit. > > > > Add check to return EOPNOTSUPP instead of EINVAL when the card is not > > able to return these information. The caller function already handles > > EOPNOTSUPP without error. > > > > So I would expect that nvm_info.num_images is 0. > > > > If not, perhaps an alternate fix is to make that so, either by setting > > it in qed_mcp_bist_nvm_get_num_images, or where the return value of > > qed_mcp_bist_nvm_get_num_images is checked (just before the goto out). > > > > Makes sense, so something like this I suppose: > > --- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c > +++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c > @@ -3301,8 +3301,10 @@ int qed_mcp_bist_nvm_get_num_images(struct qed_hwfn > *p_hwfn, > if (rc) > return rc; > > - if (((rsp & FW_MSG_CODE_MASK) == FW_MSG_CODE_UNSUPPORTED)) > + if (((rsp & FW_MSG_CODE_MASK) == FW_MSG_CODE_UNSUPPORTED)) { > + *num_images = 0; > rc = -EOPNOTSUPP; > + } > > Or the second option you stated. Yes, that is what I was thinking. But as it is a side effect, and thus hidden slightly, on reflection perhaps the second option is better. IDK. > > And, in any case I think some testing is in order. > > I strongly agree. Let me know if I can help more with this. > > Thanks for your time, > --Gian > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] qed: fix uninit pointer read in qed_mcp_nvm_info_populate() 2024-12-13 13:31 ` Simon Horman @ 2024-12-15 1:11 ` Gianfranco Trad 0 siblings, 0 replies; 5+ messages in thread From: Gianfranco Trad @ 2024-12-15 1:11 UTC (permalink / raw) To: Simon Horman Cc: manishc, andrew+netdev, davem, edumazet, kuba, pabeni, netdev, linux-kernel, skhan On 13/12/24 14:31, Simon Horman wrote: > On Fri, Dec 13, 2024 at 01:13:12PM +0100, Gianfranco Trad wrote: >> On 12/12/24 18:04, Simon Horman wrote: >>> On Wed, Dec 11, 2024 at 02:40:42PM +0100, Gianfranco Trad wrote: >>>> Coverity reports an uninit pointer read in qed_mcp_nvm_info_populate(). >>>> If qed_mcp_bist_nvm_get_num_images() returns -EOPNOTSUPP, this leads to >>>> jump to label out with nvm_info.image_att being uninit while assigning it >>>> to p_hwfn->nvm_info.image_att. >>>> Add check on rc against -EOPNOTSUPP to avoid such uninit pointer read. >>>> >>>> Closes: https://scan5.scan.coverity.com/#/project-view/63204/10063?selectedIssue=1636666 >>>> Signed-off-by: Gianfranco Trad <gianf.trad@gmail.com> >>>> --- >>>> Note: >>>> - Fixes: tag should be "7a0ea70da56e net/qed: allow old cards not supporting "num_images" to work" ? >>>> drivers/net/ethernet/qlogic/qed/qed_mcp.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/ethernet/qlogic/qed/qed_mcp.c b/drivers/net/ethernet/qlogic/qed/qed_mcp.c >>>> index b45efc272fdb..127943b39f61 100644 >>>> --- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c >>>> +++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c >>>> @@ -3387,7 +3387,7 @@ int qed_mcp_nvm_info_populate(struct qed_hwfn *p_hwfn) >>>> } >>>> out: >>>> /* Update hwfn's nvm_info */ >>>> - if (nvm_info.num_images) { >>>> + if (nvm_info.num_images && rc != -EOPNOTSUPP) { >>>> p_hwfn->nvm_info.num_images = nvm_info.num_images; >>>> kfree(p_hwfn->nvm_info.image_att); >>>> p_hwfn->nvm_info.image_att = nvm_info.image_att; >>> >> >> Hi Simon, >> >>> Are you sure that nvm_info.num_images can be non-zero if rc == -EOPNOTSUPP? >>> >> >> In the coverity report, the static analyzer is able to take the true branch >> on nvm_info.num_images. I didn't physically reproduce this logical state as >> I don't possess the matching hardware. >> >>> The cited commit state: >>> >>> Commit 43645ce03e00 ("qed: Populate nvm image attribute shadow.") >>> added support for populating flash image attributes, notably >>> "num_images". However, some cards were not able to return this >>> information. In such cases, the driver would return EINVAL, causing the >>> driver to exit. >>> >>> Add check to return EOPNOTSUPP instead of EINVAL when the card is not >>> able to return these information. The caller function already handles >>> EOPNOTSUPP without error. >>> >>> So I would expect that nvm_info.num_images is 0. >>> >>> If not, perhaps an alternate fix is to make that so, either by setting >>> it in qed_mcp_bist_nvm_get_num_images, or where the return value of >>> qed_mcp_bist_nvm_get_num_images is checked (just before the goto out). >>> >> >> Makes sense, so something like this I suppose: >> >> --- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c >> +++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c >> @@ -3301,8 +3301,10 @@ int qed_mcp_bist_nvm_get_num_images(struct qed_hwfn >> *p_hwfn, >> if (rc) >> return rc; >> >> - if (((rsp & FW_MSG_CODE_MASK) == FW_MSG_CODE_UNSUPPORTED)) >> + if (((rsp & FW_MSG_CODE_MASK) == FW_MSG_CODE_UNSUPPORTED)) { >> + *num_images = 0; >> rc = -EOPNOTSUPP; >> + } >> >> Or the second option you stated. > > Yes, that is what I was thinking. > But as it is a side effect, and thus hidden slightly, > on reflection perhaps the second option is better. IDK. > Got it. Will send a v2 accordingly. Thanks for your time, --Gian >>> And, in any case I think some testing is in order. >> >> I strongly agree. Let me know if I can help more with this. >> >> Thanks for your time, >> --Gian >> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-12-15 1:11 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-12-11 13:40 [PATCH] qed: fix uninit pointer read in qed_mcp_nvm_info_populate() Gianfranco Trad 2024-12-12 17:04 ` Simon Horman 2024-12-13 12:13 ` Gianfranco Trad 2024-12-13 13:31 ` Simon Horman 2024-12-15 1:11 ` Gianfranco Trad
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).