[bug report] asix: fix uninit-value in asix_mdio_read()

[bug report] asix: fix uninit-value in asix_mdio_read()

[Dan Carpenter]

Hello Pavel Skripkin,

The patch 8035b1a2a37a: "asix: fix uninit-value in asix_mdio_read()"
from Dec 21, 2021, leads to the following Smatch static checker
warning:

	drivers/net/usb/asix_common.c:82 asix_check_host_enable()
	warn: 'ret' possible negative type promoted to high

drivers/net/usb/asix_common.c
    68 static int asix_check_host_enable(struct usbnet *dev, int in_pm)
    69 {
    70         int i, ret;
    71         u8 smsr;
    72 
    73         for (i = 0; i < AX_HOST_EN_RETRIES; ++i) {
    74                 ret = asix_set_sw_mii(dev, in_pm);
    75                 if (ret == -ENODEV || ret == -ETIMEDOUT)
    76                         break;
    77                 usleep_range(1000, 1100);
    78                 ret = asix_read_cmd(dev, AX_CMD_STATMNGSTS_REG,
    79                                     0, 0, 1, &smsr, in_pm);
    80                 if (ret == -ENODEV)
    81                         break;
--> 82                 else if (ret < sizeof(smsr))

This has to be: if (ret < 0 || ret < sizeof(smsr)) { but even better
would be to fix asix_read_cmd() to not allow partial reads.  It should
print the netdev_warn() for partial reads but right now it doesn't.

    83                         continue;
    84                 else if (smsr & AX_HOST_EN)
    85                         break;
    86         }
    87 
    88         return i >= AX_HOST_EN_RETRIES ? -ETIMEDOUT : ret;
    89 }

regards,
dan carpenter

Re: [bug report] asix: fix uninit-value in asix_mdio_read()

[Pavel Skripkin]

Hi Dan,

On 1/6/22 13:05, Dan Carpenter wrote:
> Hello Pavel Skripkin,
> 
> The patch 8035b1a2a37a: "asix: fix uninit-value in asix_mdio_read()"
> from Dec 21, 2021, leads to the following Smatch static checker
> warning:
> 
> 	drivers/net/usb/asix_common.c:82 asix_check_host_enable()
> 	warn: 'ret' possible negative type promoted to high
> 
> drivers/net/usb/asix_common.c
>      68 static int asix_check_host_enable(struct usbnet *dev, int in_pm)
>      69 {
>      70         int i, ret;
>      71         u8 smsr;
>      72
>      73         for (i = 0; i < AX_HOST_EN_RETRIES; ++i) {
>      74                 ret = asix_set_sw_mii(dev, in_pm);
>      75                 if (ret == -ENODEV || ret == -ETIMEDOUT)
>      76                         break;
>      77                 usleep_range(1000, 1100);
>      78                 ret = asix_read_cmd(dev, AX_CMD_STATMNGSTS_REG,
>      79                                     0, 0, 1, &smsr, in_pm);
>      80                 if (ret == -ENODEV)
>      81                         break;
> --> 82                 else if (ret < sizeof(smsr))
> 
> This has to be: if (ret < 0 || ret < sizeof(smsr)) { but even better
> would be to fix asix_read_cmd() to not allow partial reads.  It should

Thanks for reporting this. It's indeed a bug.

I sent the fix yesterday, that disallows partial reads inside 
asix_read_cmd()

Please see

https://lore.kernel.org/all/20220105131952.15693-1-paskripkin@gmail.com/


With regards,
Pavel Skripkin