netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net: hisilicon: Fix usage of uninitialized variable in function mdio_sc_cfg_reg_write()
@ 2019-10-01 20:24 Yizhuo
  2019-10-02 21:22 ` David Miller
  0 siblings, 1 reply; 3+ messages in thread
From: Yizhuo @ 2019-10-01 20:24 UTC (permalink / raw)
  Cc: csong, zhiyunq, Yizhuo, Yisen Zhuang, Salil Mehta,
	David S. Miller, netdev, linux-kernel

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 <yzhai003@ucr.edu>
---
 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, &reg_value);
+		ret = regmap_read(mdio_dev->subctrl_vbase, st_reg, &reg_value);
+		if (ret)
+			return ret;
+
 		reg_value &= st_msk;
 		if ((!!check_st) == (!!reg_value))
 			break;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] net: hisilicon: Fix usage of uninitialized variable in function mdio_sc_cfg_reg_write()
  2019-10-01 20:24 [PATCH] net: hisilicon: Fix usage of uninitialized variable in function mdio_sc_cfg_reg_write() Yizhuo
@ 2019-10-02 21:22 ` David Miller
  2019-10-03 17:42   ` Yizhuo Zhai
  0 siblings, 1 reply; 3+ messages in thread
From: David Miller @ 2019-10-02 21:22 UTC (permalink / raw)
  To: yzhai003; +Cc: csong, zhiyunq, yisen.zhuang, salil.mehta, netdev, linux-kernel

From: Yizhuo <yzhai003@ucr.edu>
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 <yzhai003@ucr.edu>

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.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] net: hisilicon: Fix usage of uninitialized variable in function mdio_sc_cfg_reg_write()
  2019-10-02 21:22 ` David Miller
@ 2019-10-03 17:42   ` Yizhuo Zhai
  0 siblings, 0 replies; 3+ messages in thread
From: Yizhuo Zhai @ 2019-10-03 17:42 UTC (permalink / raw)
  To: David Miller
  Cc: Chengyu Song, Zhiyun Qian, Yisen Zhuang, Salil Mehta, netdev,
	linux-kernel

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 <davem@davemloft.net> wrote:
>
> From: Yizhuo <yzhai003@ucr.edu>
> 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 <yzhai003@ucr.edu>
>
> 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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2019-10-03 17:42 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-01 20:24 [PATCH] net: hisilicon: Fix usage of uninitialized variable in function mdio_sc_cfg_reg_write() Yizhuo
2019-10-02 21:22 ` David Miller
2019-10-03 17:42   ` Yizhuo Zhai

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).