In function mdio_sc_cfg_reg_write(), variable "reg_value" could be
uninitialized if regmap_read() fails. However, "reg_value" is used
to decide the control flow later in the if statement, which is
potentially unsafe.
Signed-off-by: Yizhuo <[email protected]>
---
drivers/net/ethernet/hisilicon/hns_mdio.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/hisilicon/hns_mdio.c b/drivers/net/ethernet/hisilicon/hns_mdio.c
index 3e863a71c513..7df5d7d211d4 100644
--- a/drivers/net/ethernet/hisilicon/hns_mdio.c
+++ b/drivers/net/ethernet/hisilicon/hns_mdio.c
@@ -148,11 +148,15 @@ static int mdio_sc_cfg_reg_write(struct hns_mdio_device *mdio_dev,
{
u32 time_cnt;
u32 reg_value;
+ int ret;
regmap_write(mdio_dev->subctrl_vbase, cfg_reg, set_val);
for (time_cnt = MDIO_TIMEOUT; time_cnt; time_cnt--) {
- regmap_read(mdio_dev->subctrl_vbase, st_reg, ®_value);
+ ret = regmap_read(mdio_dev->subctrl_vbase, st_reg, ®_value);
+ if (ret)
+ return ret;
+
reg_value &= st_msk;
if ((!!check_st) == (!!reg_value))
break;
--
2.17.1
From: Yizhuo <[email protected]>
Date: Tue, 1 Oct 2019 13:24:39 -0700
> In function mdio_sc_cfg_reg_write(), variable "reg_value" could be
> uninitialized if regmap_read() fails. However, "reg_value" is used
> to decide the control flow later in the if statement, which is
> potentially unsafe.
>
> Signed-off-by: Yizhuo <[email protected]>
Applied, but really this is such a pervasive problem.
So much code doesn't check the return value from either regmap_read
or regmap_write.
_EVEN_ in the code you are editing, the patch context shows an unchecked
regmap_write() call.
> @@ -148,11 +148,15 @@ static int mdio_sc_cfg_reg_write(struct hns_mdio_device *mdio_dev,
> {
> u32 time_cnt;
> u32 reg_value;
> + int ret;
>
> regmap_write(mdio_dev->subctrl_vbase, cfg_reg, set_val);
^^^^^^^^^^^^
Grepping for regmap_{read,write}() shows how big an issue this is.
I don't know what to do, maybe we can work over time to add checks to
all calls and then force warnings on unchecked return values so that
the problem is not introduced in the future.
Hi David:
Thanks for your feedback. "regmap_write()" could also fail and cause
influence on the caller. If patches for "regmap_write()" are needed,
then the title could be changed from "uninitialized use" to "miss
return check".
On Wed, Oct 2, 2019 at 2:22 PM David Miller <[email protected]> wrote:
>
> From: Yizhuo <[email protected]>
> Date: Tue, 1 Oct 2019 13:24:39 -0700
>
> > In function mdio_sc_cfg_reg_write(), variable "reg_value" could be
> > uninitialized if regmap_read() fails. However, "reg_value" is used
> > to decide the control flow later in the if statement, which is
> > potentially unsafe.
> >
> > Signed-off-by: Yizhuo <[email protected]>
>
> Applied, but really this is such a pervasive problem.
>
> So much code doesn't check the return value from either regmap_read
> or regmap_write.
>
> _EVEN_ in the code you are editing, the patch context shows an unchecked
> regmap_write() call.
>
> > @@ -148,11 +148,15 @@ static int mdio_sc_cfg_reg_write(struct hns_mdio_device *mdio_dev,
> > {
> > u32 time_cnt;
> > u32 reg_value;
> > + int ret;
> >
> > regmap_write(mdio_dev->subctrl_vbase, cfg_reg, set_val);
> ^^^^^^^^^^^^
>
> Grepping for regmap_{read,write}() shows how big an issue this is.
>
> I don't know what to do, maybe we can work over time to add checks to
> all calls and then force warnings on unchecked return values so that
> the problem is not introduced in the future.
--
Kind Regards,
Yizhuo Zhai
Computer Science, Graduate Student
University of California, Riverside