netdev - Re: [PATCH] net: usb: qmi_wwan: Add error handling for usbnet_get_ethernet

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <0044ac56-b391-4e2d-8c12-9ad8a14bd625@lunn.ch>
Date: Tue, 21 Jan 2025 17:30:51 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Bjørn Mork <bjorn@...k.no>
Cc: Wentao Liang <vulab@...as.ac.cn>, Jakub Kicinski <kuba@...nel.org>,
	Daniele Palmas <dnlplm@...il.com>, Breno Leitao <leitao@...ian.org>,
	netdev@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH] net: usb: qmi_wwan: Add error handling for
 usbnet_get_ethernet_addr

On Mon, Jan 20, 2025 at 07:06:52PM +0100, Bjørn Mork wrote:
> Wentao Liang <vulab@...as.ac.cn> writes:
> 
> > In qmi_wwan_bind(), usbnet_get_ethernet_addr() is called
> > without error handling, risking unnoticed failures and
> > inconsistent behavior compared to other parts of the code.
> >
> > Fix this issue by adding an error handling for the
> > usbnet_get_ethernet_addr(), improving code robustness.
> >
> > Fixes: 423ce8caab7e ("net: usb: qmi_wwan: New driver for Huawei QMI based WWAN devices")
> > Signed-off-by: Wentao Liang <vulab@...as.ac.cn>
> > ---
> >  drivers/net/usb/qmi_wwan.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
> > index e9208a8d2bfa..7aa576bfe76b 100644
> > --- a/drivers/net/usb/qmi_wwan.c
> > +++ b/drivers/net/usb/qmi_wwan.c
> > @@ -779,7 +779,9 @@ static int qmi_wwan_bind(struct usbnet *dev, struct usb_interface *intf)
> >  	/* errors aren't fatal - we can live with the dynamic address */
> >  	if (cdc_ether && cdc_ether->wMaxSegmentSize) {
> >  		dev->hard_mtu = le16_to_cpu(cdc_ether->wMaxSegmentSize);
> > -		usbnet_get_ethernet_addr(dev, cdc_ether->iMACAddress);
> > +		status = usbnet_get_ethernet_addr(dev, cdc_ether->iMACAddress);
> > +		if (status < 0)
> > +			goto err;
> >  	}
> >  
> >  	/* claim data interface and set it up */
> 
> 
> 
> Did you read the comment?
> 
> This intentinonally ignores any errors.  I don't know how to make it
> clear anough for these AI bots to understand.  Any advice there?

The problem is not really the AI bot, but the user of the AI bot. We
often see this problem, and need to teach bot drivers that the bot is
just the start. The bot points out a potential issue, but it needs a
human developer to verify the issue and consider what the real fix is,
which might be, as in this case, a false positive and no fix is
needed.

I don't know how to really fix this issue, other than try to teach the
bot drivers to actually think.

> NAK
> 
> (and why weren't I CCed?  Noticed this by chance only...)

Probably the same issue. The bot driver does not know the processes,
has not run the get_maintainers script. This is again part of the
issue with these sorts of patched, they are often low quality and need
careful review.

	Andrew