| Message ID | 20241211134041.65860-2-gianf.trad@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | qed: fix uninit pointer read in qed_mcp_nvm_info_populate() | expand |
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.
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
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 >
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 >>
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;
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(-)