Re: kern/60592: random Mac address generation in genet Ethernet driver causes problems

The following reply was made to PR kern/60592; it has been noted by GNATS.

From: [email protected]
To: [email protected]
Cc: 
Subject: Re: kern/60592: random Mac address generation in genet Ethernet
 driver causes problems
Date: Sat, 15 Aug 2026 06:27:55 +0000

 On Fri, Aug 14, 2026 at 11:00:02PM +0000, Robert Elz via gnats wrote:
 > The following reply was made to PR kern/60592; it has been noted by GNATS.
 > 
 > From: Robert Elz <[email protected]>
 > To: [email protected]
 > Cc: 
 > Subject: Re: kern/60592: random Mac address generation in genet Ethernet driver causes problems
 > Date: Sat, 15 Aug 2026 05:57:54 +0700
 > 
 >      Date:        Fri, 14 Aug 2026 21:50:04 +0000 (UTC)
 >      From:        "[email protected] via gnats" <[email protected]>
 >      Message-ID:  <[email protected]>
 >  
 >  
 >    |         if (maclo == 0 && machi == 0) {
 >    |                 /* Create one */
 >    | -               maclo = 0x00f2 | (cprng_strong32() & 0xffff0000);
 >    | +               maclo = cprng_strong32() & 0x0000ffff;
 >    |                 machi = cprng_strong32() & 0xffff;
 >    |         }
 >    |  
 >    | -       eaddr[0] = (maclo >> 24) & 0xff;
 >    | +       eaddr[0] = (maclo >> 24) | 0xf2;
 >    |         eaddr[1] = (maclo >> 16) & 0xff;
 >    |         eaddr[2] = (maclo >>  8) & 0xff;
 >    |         eaddr[3] = (maclo >>  0) & 0xff;
 >    |
 >    |
 >    | This preserves the intent to zero out second byte and have a 0xf2
 >    | in first byte and randomize third and fourth byte in the Mac address
 >    | generated.
 >  
 >  I suspect that the correct fix (which is not that) would be to change the
 >  ">>" values to be (in order) 0 8 16 24 for the 4 addr bytes, so "maclo" is
 >  treated as a little endian value, which it was clearly intended to be.
 >  
 >  Just that (not altering the init of maclo if it wasn't set) should be all
 >  that is required.   If it is desired to allow maclo to be a big endian
 >  value when passed in rather than generated, an
 >  		else
 >  			maclo = htonl(maclo);
 >  
 >  can be added to the "if" there.    The code should respect what is given
 >  to it, when it isn't all 0, however, not force "f2" anywhere, or 0 anywhere,
 >  that would be up to wherever the passed in maclo/machi values come from
 >  to assign as desired.
 >  
 >  The code as proposed also doesn't clear the multicast bit, which it
 >  claims it should (but only when it is passed in set, in which case
 >  it shouldn't - though that would be strange indeed).
 
 the proposed code clears it actually  because that part is zeroed
 even before the bit shift starts. in cprng_strong32() & 0x0000ffff.
 
 yes you are right, consideration is needed for the the read tried by the driver
 to set the MAC address. we need to only try inserting 0xf2 only when
 it is in the random block else others gets affected. 
 
 this is what we have and being tested. will commit this if
 everything looks good.
 
 Index: sys/dev/ic/bcmgenet.c
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/ic/bcmgenet.c,v
 retrieving revision 1.23
 diff -u -r1.23 bcmgenet.c
 --- sys/dev/ic/bcmgenet.c       4 Oct 2025 04:44:20 -0000       1.23
 +++ sys/dev/ic/bcmgenet.c       15 Aug 2026 06:13:21 -0000
 @@ -977,11 +977,13 @@
                 maclo = 0x00f2 | (cprng_strong32() & 0xffff0000);
                 machi = cprng_strong32() & 0xffff;
         }
 +       else
 +               maclo = htonl(maclo);
  
 -       eaddr[0] = (maclo >> 24) & 0xff;
 -       eaddr[1] = (maclo >> 16) & 0xff;
 -       eaddr[2] = (maclo >>  8) & 0xff;
 -       eaddr[3] = (maclo >>  0) & 0xff;
 +       eaddr[0] = (maclo >> 0) | 0xff;
 +       eaddr[1] = (maclo >> 8) & 0xff;
 +       eaddr[2] = (maclo >>  16) & 0xff;
 +       eaddr[3] = (maclo >>  24) & 0xff;
         eaddr[4] = (machi >>  8) & 0xff;
         eaddr[5] = (machi >>  0) & 0xff;
  }
  
 Emmanuel