diff mbox series

[v2] net: usb: rtl8150: don't incorrectly assign random MAC addresses

Message ID 20201011173030.141582-1-anant.thazhemadam@gmail.com
State Superseded
Headers show
Series [v2] net: usb: rtl8150: don't incorrectly assign random MAC addresses | expand

Commit Message

Anant Thazhemadam Oct. 11, 2020, 5:30 p.m. UTC
In set_ethernet_addr(), if get_registers() succeeds, the ethernet address
that was read must be copied over. Otherwise, a random ethernet address
must be assigned.

get_registers() returns 0 if successful, and negative error number
otherwise. However, in set_ethernet_addr(), this return value is
incorrectly checked.

Since this return value will never be equal to sizeof(node_id), a
random MAC address will always be generated and assigned to the
device; even in cases when get_registers() is successful.

Correctly modifying the condition that checks if get_registers() was
successful or not fixes this problem, and copies the ethernet address
appropriately.

Fixes: f45a4248ea4c ("net: usb: rtl8150: set random MAC address when set_ethernet_addr() fails")
Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
---
Changes in v2:
        * Fixed the format of the Fixes tag
        * Modified the commit message to better describe the issue being 
          fixed

+CCing Stephen and linux-next, since the commit fixed isn't in the networking
tree, but is present in linux-next.

 drivers/net/usb/rtl8150.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jakub Kicinski Oct. 11, 2020, 5:59 p.m. UTC | #1
On Sun, 11 Oct 2020 23:00:30 +0530 Anant Thazhemadam wrote:
> In set_ethernet_addr(), if get_registers() succeeds, the ethernet address
> that was read must be copied over. Otherwise, a random ethernet address
> must be assigned.
> 
> get_registers() returns 0 if successful, and negative error number
> otherwise. However, in set_ethernet_addr(), this return value is
> incorrectly checked.
> 
> Since this return value will never be equal to sizeof(node_id), a
> random MAC address will always be generated and assigned to the
> device; even in cases when get_registers() is successful.
> 
> Correctly modifying the condition that checks if get_registers() was
> successful or not fixes this problem, and copies the ethernet address
> appropriately.
> 
> Fixes: f45a4248ea4c ("net: usb: rtl8150: set random MAC address when set_ethernet_addr() fails")
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>

This patch is a fix to a conflict resolution in linux-next.

linux-next is not a "real" tree, it's an integration tree used to
figure out conflicts early.

We had let Stephen know about the problem already. Please wait one
week, and if the problem is still present resend this.

Thank you.
Joe Perches Oct. 11, 2020, 6:33 p.m. UTC | #2
On Sun, 2020-10-11 at 10:59 -0700, Jakub Kicinski wrote:
> On Sun, 11 Oct 2020 23:00:30 +0530 Anant Thazhemadam wrote:
> > In set_ethernet_addr(), if get_registers() succeeds, the ethernet address
> > that was read must be copied over. Otherwise, a random ethernet address
> > must be assigned.
> > 
> > get_registers() returns 0 if successful, and negative error number
> > otherwise. However, in set_ethernet_addr(), this return value is
> > incorrectly checked.
> > 
> > Since this return value will never be equal to sizeof(node_id), a
> > random MAC address will always be generated and assigned to the
> > device; even in cases when get_registers() is successful.
> > 
> > Correctly modifying the condition that checks if get_registers() was
> > successful or not fixes this problem, and copies the ethernet address
> > appropriately.

There are many unchecked uses of set_registers and get_registers
 in this file.

If failures are really expected, then it might be better to fix
them up too.

$ git grep -w '[gs]et_registers' drivers/net/usb/rtl8150.c
drivers/net/usb/rtl8150.c:static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
drivers/net/usb/rtl8150.c:static int set_registers(rtl8150_t * dev, u16 indx, u16 size, const void *data)
drivers/net/usb/rtl8150.c:      set_registers(dev, PHYADD, sizeof(data), data);
drivers/net/usb/rtl8150.c:      set_registers(dev, PHYCNT, 1, &tmp);
drivers/net/usb/rtl8150.c:              get_registers(dev, PHYCNT, 1, data);
drivers/net/usb/rtl8150.c:              get_registers(dev, PHYDAT, 2, data);
drivers/net/usb/rtl8150.c:      set_registers(dev, PHYADD, sizeof(data), data);
drivers/net/usb/rtl8150.c:      set_registers(dev, PHYCNT, 1, &tmp);
drivers/net/usb/rtl8150.c:              get_registers(dev, PHYCNT, 1, data);
drivers/net/usb/rtl8150.c:      ret = get_registers(dev, IDR, sizeof(node_id), node_id);
drivers/net/usb/rtl8150.c:      set_registers(dev, IDR, netdev->addr_len, netdev->dev_addr);
drivers/net/usb/rtl8150.c:      get_registers(dev, CR, 1, &cr);
drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &cr);
drivers/net/usb/rtl8150.c:              set_registers(dev, IDR_EEPROM + (i * 2), 2,
drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &cr);
drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &data);
drivers/net/usb/rtl8150.c:              get_registers(dev, CR, 1, &data);
drivers/net/usb/rtl8150.c:      set_registers(dev, RCR, 1, &rcr);
drivers/net/usb/rtl8150.c:      set_registers(dev, TCR, 1, &tcr);
drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &cr);
drivers/net/usb/rtl8150.c:      get_registers(dev, MSR, 1, &msr);
drivers/net/usb/rtl8150.c:      get_registers(dev, CR, 1, &cr);
drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &cr);
drivers/net/usb/rtl8150.c:      get_registers(dev, CSCR, 2, &tmp);
drivers/net/usb/rtl8150.c:      set_registers(dev, IDR, 6, netdev->dev_addr);
drivers/net/usb/rtl8150.c:      get_registers(dev, BMCR, 2, &bmcr);
drivers/net/usb/rtl8150.c:      get_registers(dev, ANLP, 2, &lpa);
Petko Manolov Oct. 11, 2020, 7:31 p.m. UTC | #3
On 20-10-11 11:33:00, Joe Perches wrote:
> On Sun, 2020-10-11 at 10:59 -0700, Jakub Kicinski wrote:
> > On Sun, 11 Oct 2020 23:00:30 +0530 Anant Thazhemadam wrote:
> > > In set_ethernet_addr(), if get_registers() succeeds, the ethernet address
> > > that was read must be copied over. Otherwise, a random ethernet address
> > > must be assigned.
> > > 
> > > get_registers() returns 0 if successful, and negative error number
> > > otherwise. However, in set_ethernet_addr(), this return value is
> > > incorrectly checked.
> > > 
> > > Since this return value will never be equal to sizeof(node_id), a
> > > random MAC address will always be generated and assigned to the
> > > device; even in cases when get_registers() is successful.
> > > 
> > > Correctly modifying the condition that checks if get_registers() was
> > > successful or not fixes this problem, and copies the ethernet address
> > > appropriately.
> 
> There are many unchecked uses of set_registers and get_registers
>  in this file.
> 
> If failures are really expected, then it might be better to fix
> them up too.

Checking the return value of each get/set_registers() is going to be a PITA and
not very helpful.  Doing so when setting the MAC address _does_ make sense as in
that case it is not a hard error.

In almost all other occasions if usb_control_msg_send/recv() return an error i'd
rather dump an error message (from within get/set_registers()) and let the user
decide whether to get rid of this adapter or start debugging it.


cheers,
Petko


> $ git grep -w '[gs]et_registers' drivers/net/usb/rtl8150.c
> drivers/net/usb/rtl8150.c:static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
> drivers/net/usb/rtl8150.c:static int set_registers(rtl8150_t * dev, u16 indx, u16 size, const void *data)
> drivers/net/usb/rtl8150.c:      set_registers(dev, PHYADD, sizeof(data), data);
> drivers/net/usb/rtl8150.c:      set_registers(dev, PHYCNT, 1, &tmp);
> drivers/net/usb/rtl8150.c:              get_registers(dev, PHYCNT, 1, data);
> drivers/net/usb/rtl8150.c:              get_registers(dev, PHYDAT, 2, data);
> drivers/net/usb/rtl8150.c:      set_registers(dev, PHYADD, sizeof(data), data);
> drivers/net/usb/rtl8150.c:      set_registers(dev, PHYCNT, 1, &tmp);
> drivers/net/usb/rtl8150.c:              get_registers(dev, PHYCNT, 1, data);
> drivers/net/usb/rtl8150.c:      ret = get_registers(dev, IDR, sizeof(node_id), node_id);
> drivers/net/usb/rtl8150.c:      set_registers(dev, IDR, netdev->addr_len, netdev->dev_addr);
> drivers/net/usb/rtl8150.c:      get_registers(dev, CR, 1, &cr);
> drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &cr);
> drivers/net/usb/rtl8150.c:              set_registers(dev, IDR_EEPROM + (i * 2), 2,
> drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &cr);
> drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &data);
> drivers/net/usb/rtl8150.c:              get_registers(dev, CR, 1, &data);
> drivers/net/usb/rtl8150.c:      set_registers(dev, RCR, 1, &rcr);
> drivers/net/usb/rtl8150.c:      set_registers(dev, TCR, 1, &tcr);
> drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &cr);
> drivers/net/usb/rtl8150.c:      get_registers(dev, MSR, 1, &msr);
> drivers/net/usb/rtl8150.c:      get_registers(dev, CR, 1, &cr);
> drivers/net/usb/rtl8150.c:      set_registers(dev, CR, 1, &cr);
> drivers/net/usb/rtl8150.c:      get_registers(dev, CSCR, 2, &tmp);
> drivers/net/usb/rtl8150.c:      set_registers(dev, IDR, 6, netdev->dev_addr);
> drivers/net/usb/rtl8150.c:      get_registers(dev, BMCR, 2, &bmcr);
> drivers/net/usb/rtl8150.c:      get_registers(dev, ANLP, 2, &lpa);
> 
> 
>
Joe Perches Oct. 11, 2020, 8:14 p.m. UTC | #4
On Sun, 2020-10-11 at 22:31 +0300, Petko Manolov wrote:
> On 20-10-11 11:33:00, Joe Perches wrote:
> > On Sun, 2020-10-11 at 10:59 -0700, Jakub Kicinski wrote:
> > > On Sun, 11 Oct 2020 23:00:30 +0530 Anant Thazhemadam wrote:
> > > > In set_ethernet_addr(), if get_registers() succeeds, the ethernet address
> > > > that was read must be copied over. Otherwise, a random ethernet address
> > > > must be assigned.
> > > > 
> > > > get_registers() returns 0 if successful, and negative error number
> > > > otherwise. However, in set_ethernet_addr(), this return value is
> > > > incorrectly checked.
> > > > 
> > > > Since this return value will never be equal to sizeof(node_id), a
> > > > random MAC address will always be generated and assigned to the
> > > > device; even in cases when get_registers() is successful.
> > > > 
> > > > Correctly modifying the condition that checks if get_registers() was
> > > > successful or not fixes this problem, and copies the ethernet address
> > > > appropriately.
> > 
> > There are many unchecked uses of set_registers and get_registers
> >  in this file.
> > 
> > If failures are really expected, then it might be better to fix
> > them up too.
> 
> Checking the return value of each get/set_registers() is going to be a PITA and
> not very helpful.  Doing so when setting the MAC address _does_ make sense as in
> that case it is not a hard error.
> 
> In almost all other occasions if usb_control_msg_send/recv() return an error i'd
> rather dump an error message (from within get/set_registers()) and let the user
> decide whether to get rid of this adapter or start debugging it.

Your code, your choices...

Consider using _once or _ratelimited output too.
Stephen Rothwell Oct. 11, 2020, 10:14 p.m. UTC | #5
Hi Greg,

On Sun, 11 Oct 2020 23:00:30 +0530 Anant Thazhemadam <anant.thazhemadam@gmail.com> wrote:
>
> In set_ethernet_addr(), if get_registers() succeeds, the ethernet address
> that was read must be copied over. Otherwise, a random ethernet address
> must be assigned.
> 
> get_registers() returns 0 if successful, and negative error number
> otherwise. However, in set_ethernet_addr(), this return value is
> incorrectly checked.
> 
> Since this return value will never be equal to sizeof(node_id), a
> random MAC address will always be generated and assigned to the
> device; even in cases when get_registers() is successful.
> 
> Correctly modifying the condition that checks if get_registers() was
> successful or not fixes this problem, and copies the ethernet address
> appropriately.
> 
> Fixes: f45a4248ea4c ("net: usb: rtl8150: set random MAC address when set_ethernet_addr() fails")
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
> ---
> Changes in v2:
>         * Fixed the format of the Fixes tag
>         * Modified the commit message to better describe the issue being 
>           fixed
> 
> +CCing Stephen and linux-next, since the commit fixed isn't in the networking
> tree, but is present in linux-next.
> 
>  drivers/net/usb/rtl8150.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index f020401adf04..bf8a60533f3e 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -261,7 +261,7 @@ static void set_ethernet_addr(rtl8150_t *dev)
>  
>  	ret = get_registers(dev, IDR, sizeof(node_id), node_id);
>  
> -	if (ret == sizeof(node_id)) {
> +	if (!ret) {
>  		ether_addr_copy(dev->netdev->dev_addr, node_id);
>  	} else {
>  		eth_hw_addr_random(dev->netdev);
> -- 
> 2.25.1
> 

I will apply the above patch to the merge of the usb tree today to fix
up a semantic conflict between the usb tree and Linus' tree.
Stephen Rothwell Oct. 15, 2020, 9:59 p.m. UTC | #6
Hi Greg,

On Mon, 12 Oct 2020 09:14:28 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> On Sun, 11 Oct 2020 23:00:30 +0530 Anant Thazhemadam <anant.thazhemadam@gmail.com> wrote:
> >
> > In set_ethernet_addr(), if get_registers() succeeds, the ethernet address
> > that was read must be copied over. Otherwise, a random ethernet address
> > must be assigned.
> > 
> > get_registers() returns 0 if successful, and negative error number
> > otherwise. However, in set_ethernet_addr(), this return value is
> > incorrectly checked.
> > 
> > Since this return value will never be equal to sizeof(node_id), a
> > random MAC address will always be generated and assigned to the
> > device; even in cases when get_registers() is successful.
> > 
> > Correctly modifying the condition that checks if get_registers() was
> > successful or not fixes this problem, and copies the ethernet address
> > appropriately.
> > 
> > Fixes: f45a4248ea4c ("net: usb: rtl8150: set random MAC address when set_ethernet_addr() fails")
> > Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
> > ---
> > Changes in v2:
> >         * Fixed the format of the Fixes tag
> >         * Modified the commit message to better describe the issue being 
> >           fixed
> > 
> > +CCing Stephen and linux-next, since the commit fixed isn't in the networking
> > tree, but is present in linux-next.
> > 
> >  drivers/net/usb/rtl8150.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> > index f020401adf04..bf8a60533f3e 100644
> > --- a/drivers/net/usb/rtl8150.c
> > +++ b/drivers/net/usb/rtl8150.c
> > @@ -261,7 +261,7 @@ static void set_ethernet_addr(rtl8150_t *dev)
> >  
> >  	ret = get_registers(dev, IDR, sizeof(node_id), node_id);
> >  
> > -	if (ret == sizeof(node_id)) {
> > +	if (!ret) {
> >  		ether_addr_copy(dev->netdev->dev_addr, node_id);
> >  	} else {
> >  		eth_hw_addr_random(dev->netdev);
> > -- 
> > 2.25.1
> >   
> 
> I will apply the above patch to the merge of the usb tree today to fix
> up a semantic conflict between the usb tree and Linus' tree.

It looks like you forgot to mention this one to Linus :-(

It should probably say:

Fixes: b2a0f274e3f7 ("net: rtl8150: Use the new usb control message API.")
Jakub Kicinski Oct. 15, 2020, 10:24 p.m. UTC | #7
On Fri, 16 Oct 2020 08:59:22 +1100 Stephen Rothwell wrote:
> > I will apply the above patch to the merge of the usb tree today to fix
> > up a semantic conflict between the usb tree and Linus' tree.  
> 
> It looks like you forgot to mention this one to Linus :-(
> 
> It should probably say:
> 
> Fixes: b2a0f274e3f7 ("net: rtl8150: Use the new usb control message API.")

Umpf. I think Greg sent his changes first, so this is on me.

The networking PR is still outstanding, can we reply to the PR with
the merge guidance. Or is it too late?
Jakub Kicinski Oct. 15, 2020, 10:37 p.m. UTC | #8
On Thu, 15 Oct 2020 15:24:51 -0700 Jakub Kicinski wrote:
> On Fri, 16 Oct 2020 08:59:22 +1100 Stephen Rothwell wrote:
> > > I will apply the above patch to the merge of the usb tree today to fix
> > > up a semantic conflict between the usb tree and Linus' tree.    
> > 
> > It looks like you forgot to mention this one to Linus :-(
> > 
> > It should probably say:
> > 
> > Fixes: b2a0f274e3f7 ("net: rtl8150: Use the new usb control message API.")  
> 
> Umpf. I think Greg sent his changes first, so this is on me.
> 
> The networking PR is still outstanding, can we reply to the PR with
> the merge guidance. Or is it too late?

I take that back, this came through net, not net-next so our current
PR is irrelevant :)
Jakub Kicinski Oct. 18, 2020, 7:54 p.m. UTC | #9
On Thu, 15 Oct 2020 15:37:00 -0700 Jakub Kicinski wrote:
> On Thu, 15 Oct 2020 15:24:51 -0700 Jakub Kicinski wrote:
> > On Fri, 16 Oct 2020 08:59:22 +1100 Stephen Rothwell wrote:  
> > > > I will apply the above patch to the merge of the usb tree today to fix
> > > > up a semantic conflict between the usb tree and Linus' tree.      
> > > 
> > > It looks like you forgot to mention this one to Linus :-(
> > > 
> > > It should probably say:
> > > 
> > > Fixes: b2a0f274e3f7 ("net: rtl8150: Use the new usb control message API.")    
> > 
> > Umpf. I think Greg sent his changes first, so this is on me.
> > 
> > The networking PR is still outstanding, can we reply to the PR with
> > the merge guidance. Or is it too late?  
> 
> I take that back, this came through net, not net-next so our current
> PR is irrelevant :)

Looks like the best thing to do right now is to put this fix in net,
so let me do just that (with the Fixes tag from Stephen).

Thanks Anant!
diff mbox series

Patch

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index f020401adf04..bf8a60533f3e 100644
--- a/drivers/net/usb/rtl8150.c
+++ b/drivers/net/usb/rtl8150.c
@@ -261,7 +261,7 @@  static void set_ethernet_addr(rtl8150_t *dev)
 
 	ret = get_registers(dev, IDR, sizeof(node_id), node_id);
 
-	if (ret == sizeof(node_id)) {
+	if (!ret) {
 		ether_addr_copy(dev->netdev->dev_addr, node_id);
 	} else {
 		eth_hw_addr_random(dev->netdev);