Re: svn commit: r360068 - in head/sys: kern net sys

2020-04-21 Thread Kristof Provost

On 21 Apr 2020, at 4:34, Kyle Evans wrote:

On Mon, Apr 20, 2020 at 9:14 PM Kyle Evans  wrote:


On Mon, Apr 20, 2020 at 8:15 PM Eric van Gyzen  
wrote:



+  sz = asprintf(, M_TEMP, "%s-%s-%s", uuid, if_name(ifp),
+  jailname);
+  if (sz < 0) {
+  /* Fall back to a random mac address. */



I was wondering if it would be valuable to give this fall back 
something

like:

printf("%s: unable to create fixed mac address; using 
random

mac address", if_name(ifp));

This will only be printed in rare circumstances. But in that case 
will

provide valuable information.

That would potentially be valuable, yes. On the other hand, we 
traditionally
don???t sprinkle a lot of printf()s around in the kernel. This is 
extremely
unlikely to happen, and if it does odds are attaching the 
interface will
fail at an earlier or later point, you may struggle to pass 
packets and run

into any number of other issues.
It???s also possible to diagnose absent the printf(), because the 
MAC
address will be locally administered rather than within the 
FreeBSD OUI.


So, in short: not a bad idea. You can argue it both ways, and I 
find myself

(weakly) on the opposite side.


Would displaying the message only when verbose boot mode is enabled 
be

a suitable compromise?


We could completely avoid the problems of dynamic allocation by 
calling

SHA1Update three times, feeding each piece of data separately.

For bonus points, use a single char[] to save stack space, too.  
Maybe
use a union, for legibility, and to ensure the proper size without 
ugly

assertions.



To be honest, I'd be more inclined to just revert this part of it and
push it all back onto the stack. It's still < 512 bytes and pretty
much always called in short paths because it's generally only used
during initial creation of some ifnet; I found the concern about the
stack usage here, specifically, a bit dubious in the first place, and
this follow-up hasn't left me enjoying it any further.



Sorry, to clarify: I'm also pretty much OK with SHA1Update 3x if I'm
alone in the "don't really care about this particular stack usage"
camp, but I've found it useful that they're currently joined into a
single buffer as I've had occasion to dump it in the past to confirm
my understanding of the pedigree of the output, in case of, e.g.,
generated conflicts.


For what it’s worth, I’m in your camp: a few hundred bytes of stack 
use doesn’t matter much here. Straightforward code is more important.


Best regards,
Kristof
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"


Re: svn commit: r360068 - in head/sys: kern net sys

2020-04-20 Thread Kyle Evans
On Mon, Apr 20, 2020 at 9:14 PM Kyle Evans  wrote:
>
> On Mon, Apr 20, 2020 at 8:15 PM Eric van Gyzen  wrote:
> >
> >  +  sz = asprintf(, M_TEMP, "%s-%s-%s", uuid, if_name(ifp),
> >  +  jailname);
> >  +  if (sz < 0) {
> >  +  /* Fall back to a random mac address. */
> > >>>
> > >>>
> > >>> I was wondering if it would be valuable to give this fall back something
> > >>> like:
> > >>>
> > >>> printf("%s: unable to create fixed mac address; using random
> > >>> mac address", if_name(ifp));
> > >>>
> > >>> This will only be printed in rare circumstances. But in that case will
> > >>> provide valuable information.
> > >>>
> > >> That would potentially be valuable, yes. On the other hand, we 
> > >> traditionally
> > >> don???t sprinkle a lot of printf()s around in the kernel. This is 
> > >> extremely
> > >> unlikely to happen, and if it does odds are attaching the interface will
> > >> fail at an earlier or later point, you may struggle to pass packets and 
> > >> run
> > >> into any number of other issues.
> > >> It???s also possible to diagnose absent the printf(), because the MAC
> > >> address will be locally administered rather than within the FreeBSD OUI.
> > >>
> > >> So, in short: not a bad idea. You can argue it both ways, and I find 
> > >> myself
> > >> (weakly) on the opposite side.
> > >
> > > Would displaying the message only when verbose boot mode is enabled be
> > > a suitable compromise?
> >
> > We could completely avoid the problems of dynamic allocation by calling
> > SHA1Update three times, feeding each piece of data separately.
> >
> > For bonus points, use a single char[] to save stack space, too.  Maybe
> > use a union, for legibility, and to ensure the proper size without ugly
> > assertions.
> >
>
> To be honest, I'd be more inclined to just revert this part of it and
> push it all back onto the stack. It's still < 512 bytes and pretty
> much always called in short paths because it's generally only used
> during initial creation of some ifnet; I found the concern about the
> stack usage here, specifically, a bit dubious in the first place, and
> this follow-up hasn't left me enjoying it any further.
>

Sorry, to clarify: I'm also pretty much OK with SHA1Update 3x if I'm
alone in the "don't really care about this particular stack usage"
camp, but I've found it useful that they're currently joined into a
single buffer as I've had occasion to dump it in the past to confirm
my understanding of the pedigree of the output, in case of, e.g.,
generated conflicts.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"


Re: svn commit: r360068 - in head/sys: kern net sys

2020-04-20 Thread Kyle Evans
On Mon, Apr 20, 2020 at 8:15 PM Eric van Gyzen  wrote:
>
>  +  sz = asprintf(, M_TEMP, "%s-%s-%s", uuid, if_name(ifp),
>  +  jailname);
>  +  if (sz < 0) {
>  +  /* Fall back to a random mac address. */
> >>>
> >>>
> >>> I was wondering if it would be valuable to give this fall back something
> >>> like:
> >>>
> >>> printf("%s: unable to create fixed mac address; using random
> >>> mac address", if_name(ifp));
> >>>
> >>> This will only be printed in rare circumstances. But in that case will
> >>> provide valuable information.
> >>>
> >> That would potentially be valuable, yes. On the other hand, we 
> >> traditionally
> >> don???t sprinkle a lot of printf()s around in the kernel. This is extremely
> >> unlikely to happen, and if it does odds are attaching the interface will
> >> fail at an earlier or later point, you may struggle to pass packets and run
> >> into any number of other issues.
> >> It???s also possible to diagnose absent the printf(), because the MAC
> >> address will be locally administered rather than within the FreeBSD OUI.
> >>
> >> So, in short: not a bad idea. You can argue it both ways, and I find myself
> >> (weakly) on the opposite side.
> >
> > Would displaying the message only when verbose boot mode is enabled be
> > a suitable compromise?
>
> We could completely avoid the problems of dynamic allocation by calling
> SHA1Update three times, feeding each piece of data separately.
>
> For bonus points, use a single char[] to save stack space, too.  Maybe
> use a union, for legibility, and to ensure the proper size without ugly
> assertions.
>

To be honest, I'd be more inclined to just revert this part of it and
push it all back onto the stack. It's still < 512 bytes and pretty
much always called in short paths because it's generally only used
during initial creation of some ifnet; I found the concern about the
stack usage here, specifically, a bit dubious in the first place, and
this follow-up hasn't left me enjoying it any further.

Thanks,

Kyle Evans
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"


Re: svn commit: r360068 - in head/sys: kern net sys

2020-04-20 Thread Eric van Gyzen

+   sz = asprintf(, M_TEMP, "%s-%s-%s", uuid, if_name(ifp),
+   jailname);
+   if (sz < 0) {
+   /* Fall back to a random mac address. */



I was wondering if it would be valuable to give this fall back something
like:

printf("%s: unable to create fixed mac address; using random
mac address", if_name(ifp));

This will only be printed in rare circumstances. But in that case will
provide valuable information.


That would potentially be valuable, yes. On the other hand, we traditionally
don???t sprinkle a lot of printf()s around in the kernel. This is extremely
unlikely to happen, and if it does odds are attaching the interface will
fail at an earlier or later point, you may struggle to pass packets and run
into any number of other issues.
It???s also possible to diagnose absent the printf(), because the MAC
address will be locally administered rather than within the FreeBSD OUI.

So, in short: not a bad idea. You can argue it both ways, and I find myself
(weakly) on the opposite side.


Would displaying the message only when verbose boot mode is enabled be
a suitable compromise?


We could completely avoid the problems of dynamic allocation by calling 
SHA1Update three times, feeding each piece of data separately.


For bonus points, use a single char[] to save stack space, too.  Maybe 
use a union, for legibility, and to ensure the proper size without ugly 
assertions.


Eric
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"


Re: svn commit: r360068 - in head/sys: kern net sys

2020-04-19 Thread Shawn Webb
On Sun, Apr 19, 2020 at 04:19:06PM +0200, Kristof Provost wrote:
> On 19 Apr 2020, at 15:33, Ronald Klop wrote:
> > On Sat, 18 Apr 2020 09:50:30 +0200, Kristof Provost 
> > wrote:
> > 
> > > Author: kp
> > > Date: Sat Apr 18 07:50:30 2020
> > > New Revision: 360068
> > > URL: https://svnweb.freebsd.org/changeset/base/360068
> > > 
> > > Log:
> > >   ethersubr: Make the mac address generation more robust
> > >  If we create two (vnet) jails and create a bridge interface in each
> > > we end up
> > >   with the same mac address on both bridge interfaces.
> > >   These very often conflicts, resulting in same mac address in both
> > > jails.
> > >  Mitigate this problem by including the jail name in the mac address.
> > >  Reviewed by: kevans, melifaro
> > >   MFC after:  1 week
> > >   Differential Revision:  https://reviews.freebsd.org/D24383
> > > 
> > > Modified:
> > >   head/sys/kern/kern_jail.c
> > >   head/sys/net/if_ethersubr.c
> > >   head/sys/sys/jail.h
> > > 
> > > Modified: head/sys/kern/kern_jail.c
> > > ==
> > > --- head/sys/kern/kern_jail.c Sat Apr 18 03:14:16 2020
> > > (r360067)
> > > +++ head/sys/kern/kern_jail.c Sat Apr 18 07:50:30 2020
> > > (r360068)
> > > @@ -2920,6 +2920,15 @@ getcredhostid(struct ucred *cred, unsigned
> > > long *hosti
> > >   mtx_unlock(>cr_prison->pr_mtx);
> > >  }
> > > +void
> > > +getjailname(struct ucred *cred, char *name, size_t len)
> > > +{
> > > +
> > > + mtx_lock(>cr_prison->pr_mtx);
> > > + strlcpy(name, cred->cr_prison->pr_name, len);
> > > + mtx_unlock(>cr_prison->pr_mtx);
> > > +}
> > > +
> > >  #ifdef VIMAGE
> > >  /*
> > >   * Determine whether the prison represented by cred owns
> > > 
> > > Modified: head/sys/net/if_ethersubr.c
> > > ==
> > > --- head/sys/net/if_ethersubr.c   Sat Apr 18 03:14:16 2020
> > > (r360067)
> > > +++ head/sys/net/if_ethersubr.c   Sat Apr 18 07:50:30 2020
> > > (r360068)
> > > @@ -1419,27 +1419,39 @@ ether_8021q_frame(struct mbuf **mp, struct
> > > ifnet *ife,
> > > /*
> > >   * Allocate an address from the FreeBSD Foundation OUI.  This uses a
> > > - * cryptographic hash function on the containing jail's UUID and
> > > the interface
> > > - * name to attempt to provide a unique but stable address.
> > > Pseudo-interfaces
> > > - * which require a MAC address should use this function to allocate
> > > - * non-locally-administered addresses.
> > > + * cryptographic hash function on the containing jail's name, UUID
> > > and the
> > > + * interface name to attempt to provide a unique but stable address.
> > > + * Pseudo-interfaces which require a MAC address should use this
> > > function to
> > > + * allocate non-locally-administered addresses.
> > >   */
> > >  void
> > >  ether_gen_addr(struct ifnet *ifp, struct ether_addr *hwaddr)
> > >  {
> > > -#define  ETHER_GEN_ADDR_BUFSIZ   HOSTUUIDLEN + IFNAMSIZ + 2
> > >   SHA1_CTX ctx;
> > > - char buf[ETHER_GEN_ADDR_BUFSIZ];
> > > + char *buf;
> > >   char uuid[HOSTUUIDLEN + 1];
> > >   uint64_t addr;
> > >   int i, sz;
> > >   char digest[SHA1_RESULTLEN];
> > > + char jailname[MAXHOSTNAMELEN];
> > >   getcredhostuuid(curthread->td_ucred, uuid, sizeof(uuid));
> > > - sz = snprintf(buf, ETHER_GEN_ADDR_BUFSIZ, "%s-%s", uuid,
> > > ifp->if_xname);
> > > + /* If each (vnet) jail would also have a unique hostuuid this
> > > would not
> > > +  * be necessary. */
> > > + getjailname(curthread->td_ucred, jailname, sizeof(jailname));
> > > + sz = asprintf(, M_TEMP, "%s-%s-%s", uuid, if_name(ifp),
> > > + jailname);
> > > + if (sz < 0) {
> > > + /* Fall back to a random mac address. */
> > 
> > 
> > I was wondering if it would be valuable to give this fall back something
> > like:
> > 
> >printf("%s: unable to create fixed mac address; using random
> > mac address", if_name(ifp));
> > 
> > This will only be printed in rare circumstances. But in that case will
> > provide valuable information.
> > 
> That would potentially be valuable, yes. On the other hand, we traditionally
> don???t sprinkle a lot of printf()s around in the kernel. This is extremely
> unlikely to happen, and if it does odds are attaching the interface will
> fail at an earlier or later point, you may struggle to pass packets and run
> into any number of other issues.
> It???s also possible to diagnose absent the printf(), because the MAC
> address will be locally administered rather than within the FreeBSD OUI.
> 
> So, in short: not a bad idea. You can argue it both ways, and I find myself
> (weakly) on the opposite side.

Would displaying the message only when verbose boot mode is enabled be
a suitable compromise?

Thanks,

-- 
Shawn Webb
Cofounder / Security Engineer
HardenedBSD

GPG Key ID:  0xFF2E67A277F8E1FA
GPG Key Fingerprint: D206 BB45 15E0 9C49 0CF9  3633 C85B 0AF8 AB23 0FB2

Re: svn commit: r360068 - in head/sys: kern net sys

2020-04-19 Thread Kristof Provost

On 19 Apr 2020, at 15:33, Ronald Klop wrote:
On Sat, 18 Apr 2020 09:50:30 +0200, Kristof Provost  
wrote:



Author: kp
Date: Sat Apr 18 07:50:30 2020
New Revision: 360068
URL: https://svnweb.freebsd.org/changeset/base/360068

Log:
  ethersubr: Make the mac address generation more robust
 If we create two (vnet) jails and create a bridge interface in each 
we end up

  with the same mac address on both bridge interfaces.
  These very often conflicts, resulting in same mac address in both 
jails.

 Mitigate this problem by including the jail name in the mac address.
 Reviewed by:   kevans, melifaro
  MFC after:1 week
  Differential Revision:https://reviews.freebsd.org/D24383

Modified:
  head/sys/kern/kern_jail.c
  head/sys/net/if_ethersubr.c
  head/sys/sys/jail.h

Modified: head/sys/kern/kern_jail.c
==
--- head/sys/kern/kern_jail.c   Sat Apr 18 03:14:16 2020(r360067)
+++ head/sys/kern/kern_jail.c   Sat Apr 18 07:50:30 2020(r360068)
@@ -2920,6 +2920,15 @@ getcredhostid(struct ucred *cred, unsigned 
long *hosti

mtx_unlock(>cr_prison->pr_mtx);
 }
+void
+getjailname(struct ucred *cred, char *name, size_t len)
+{
+
+   mtx_lock(>cr_prison->pr_mtx);
+   strlcpy(name, cred->cr_prison->pr_name, len);
+   mtx_unlock(>cr_prison->pr_mtx);
+}
+
 #ifdef VIMAGE
 /*
  * Determine whether the prison represented by cred owns

Modified: head/sys/net/if_ethersubr.c
==
--- head/sys/net/if_ethersubr.c Sat Apr 18 03:14:16 2020(r360067)
+++ head/sys/net/if_ethersubr.c Sat Apr 18 07:50:30 2020(r360068)
@@ -1419,27 +1419,39 @@ ether_8021q_frame(struct mbuf **mp, struct 
ifnet *ife,

/*
  * Allocate an address from the FreeBSD Foundation OUI.  This uses a
- * cryptographic hash function on the containing jail's UUID and the 
interface
- * name to attempt to provide a unique but stable address.  
Pseudo-interfaces

- * which require a MAC address should use this function to allocate
- * non-locally-administered addresses.
+ * cryptographic hash function on the containing jail's name, UUID 
and the

+ * interface name to attempt to provide a unique but stable address.
+ * Pseudo-interfaces which require a MAC address should use this 
function to

+ * allocate non-locally-administered addresses.
  */
 void
 ether_gen_addr(struct ifnet *ifp, struct ether_addr *hwaddr)
 {
-#defineETHER_GEN_ADDR_BUFSIZ   HOSTUUIDLEN + IFNAMSIZ + 2
SHA1_CTX ctx;
-   char buf[ETHER_GEN_ADDR_BUFSIZ];
+   char *buf;
char uuid[HOSTUUIDLEN + 1];
uint64_t addr;
int i, sz;
char digest[SHA1_RESULTLEN];
+   char jailname[MAXHOSTNAMELEN];
getcredhostuuid(curthread->td_ucred, uuid, sizeof(uuid));
-	sz = snprintf(buf, ETHER_GEN_ADDR_BUFSIZ, "%s-%s", uuid, 
ifp->if_xname);
+	/* If each (vnet) jail would also have a unique hostuuid this would 
not

+* be necessary. */
+   getjailname(curthread->td_ucred, jailname, sizeof(jailname));
+   sz = asprintf(, M_TEMP, "%s-%s-%s", uuid, if_name(ifp),
+   jailname);
+   if (sz < 0) {
+   /* Fall back to a random mac address. */



I was wondering if it would be valuable to give this fall back 
something like:


   printf("%s: unable to create fixed mac address; using 
random mac address", if_name(ifp));


This will only be printed in rare circumstances. But in that case will 
provide valuable information.


That would potentially be valuable, yes. On the other hand, we 
traditionally don’t sprinkle a lot of printf()s around in the kernel. 
This is extremely unlikely to happen, and if it does odds are attaching 
the interface will fail at an earlier or later point, you may struggle 
to pass packets and run into any number of other issues.
It’s also possible to diagnose absent the printf(), because the MAC 
address will be locally administered rather than within the FreeBSD OUI.


So, in short: not a bad idea. You can argue it both ways, and I find 
myself (weakly) on the opposite side.


Best regards,
Kristof
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"


Re: svn commit: r360068 - in head/sys: kern net sys

2020-04-19 Thread Ronald Klop

Nice feature. A question below.


On Sat, 18 Apr 2020 09:50:30 +0200, Kristof Provost  wrote:


Author: kp
Date: Sat Apr 18 07:50:30 2020
New Revision: 360068
URL: https://svnweb.freebsd.org/changeset/base/360068

Log:
  ethersubr: Make the mac address generation more robust
 If we create two (vnet) jails and create a bridge interface in each we  
end up

  with the same mac address on both bridge interfaces.
  These very often conflicts, resulting in same mac address in both  
jails.

 Mitigate this problem by including the jail name in the mac address.
 Reviewed by:   kevans, melifaro
  MFC after:1 week
  Differential Revision:https://reviews.freebsd.org/D24383

Modified:
  head/sys/kern/kern_jail.c
  head/sys/net/if_ethersubr.c
  head/sys/sys/jail.h

Modified: head/sys/kern/kern_jail.c
==
--- head/sys/kern/kern_jail.c   Sat Apr 18 03:14:16 2020(r360067)
+++ head/sys/kern/kern_jail.c   Sat Apr 18 07:50:30 2020(r360068)
@@ -2920,6 +2920,15 @@ getcredhostid(struct ucred *cred, unsigned long  
*hosti

mtx_unlock(>cr_prison->pr_mtx);
 }
+void
+getjailname(struct ucred *cred, char *name, size_t len)
+{
+
+   mtx_lock(>cr_prison->pr_mtx);
+   strlcpy(name, cred->cr_prison->pr_name, len);
+   mtx_unlock(>cr_prison->pr_mtx);
+}
+
 #ifdef VIMAGE
 /*
  * Determine whether the prison represented by cred owns

Modified: head/sys/net/if_ethersubr.c
==
--- head/sys/net/if_ethersubr.c Sat Apr 18 03:14:16 2020(r360067)
+++ head/sys/net/if_ethersubr.c Sat Apr 18 07:50:30 2020(r360068)
@@ -1419,27 +1419,39 @@ ether_8021q_frame(struct mbuf **mp, struct ifnet  
*ife,

/*
  * Allocate an address from the FreeBSD Foundation OUI.  This uses a
- * cryptographic hash function on the containing jail's UUID and the  
interface
- * name to attempt to provide a unique but stable address.   
Pseudo-interfaces

- * which require a MAC address should use this function to allocate
- * non-locally-administered addresses.
+ * cryptographic hash function on the containing jail's name, UUID and  
the

+ * interface name to attempt to provide a unique but stable address.
+ * Pseudo-interfaces which require a MAC address should use this  
function to

+ * allocate non-locally-administered addresses.
  */
 void
 ether_gen_addr(struct ifnet *ifp, struct ether_addr *hwaddr)
 {
-#defineETHER_GEN_ADDR_BUFSIZ   HOSTUUIDLEN + IFNAMSIZ + 2
SHA1_CTX ctx;
-   char buf[ETHER_GEN_ADDR_BUFSIZ];
+   char *buf;
char uuid[HOSTUUIDLEN + 1];
uint64_t addr;
int i, sz;
char digest[SHA1_RESULTLEN];
+   char jailname[MAXHOSTNAMELEN];
getcredhostuuid(curthread->td_ucred, uuid, sizeof(uuid));
-	sz = snprintf(buf, ETHER_GEN_ADDR_BUFSIZ, "%s-%s", uuid,  
ifp->if_xname);

+   /* If each (vnet) jail would also have a unique hostuuid this would not
+* be necessary. */
+   getjailname(curthread->td_ucred, jailname, sizeof(jailname));
+   sz = asprintf(, M_TEMP, "%s-%s-%s", uuid, if_name(ifp),
+   jailname);
+   if (sz < 0) {
+   /* Fall back to a random mac address. */



I was wondering if it would be valuable to give this fall back something  
like:


   printf("%s: unable to create fixed mac address; using random  
mac address", if_name(ifp));


This will only be printed in rare circumstances. But in that case will  
provide valuable information.


Regards,

Ronald.



+   arc4rand(hwaddr, sizeof(*hwaddr), 0);
+   hwaddr->octet[0] = 0x02;
+   return;
+   }
+
SHA1Init();
SHA1Update(, buf, sz);
SHA1Final(digest, );
+   free(buf, M_TEMP);
addr = ((digest[0] << 16) | (digest[1] << 8) | digest[2]) &
OUI_FREEBSD_GENERATED_MASK;

Modified: head/sys/sys/jail.h
==
--- head/sys/sys/jail.h Sat Apr 18 03:14:16 2020(r360067)
+++ head/sys/sys/jail.h Sat Apr 18 07:50:30 2020(r360068)
@@ -382,6 +382,7 @@ void getcredhostname(struct ucred *, char *, size_t);
 void getcreddomainname(struct ucred *, char *, size_t);
 void getcredhostuuid(struct ucred *, char *, size_t);
 void getcredhostid(struct ucred *, unsigned long *);
+void getjailname(struct ucred *cred, char *name, size_t len);
 void prison0_init(void);
 int prison_allow(struct ucred *, unsigned);
 int prison_check(struct ucred *cred1, struct ucred *cred2);
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send 

svn commit: r360068 - in head/sys: kern net sys

2020-04-18 Thread Kristof Provost
Author: kp
Date: Sat Apr 18 07:50:30 2020
New Revision: 360068
URL: https://svnweb.freebsd.org/changeset/base/360068

Log:
  ethersubr: Make the mac address generation more robust
  
  If we create two (vnet) jails and create a bridge interface in each we end up
  with the same mac address on both bridge interfaces.
  These very often conflicts, resulting in same mac address in both jails.
  
  Mitigate this problem by including the jail name in the mac address.
  
  Reviewed by:  kevans, melifaro
  MFC after:1 week
  Differential Revision:https://reviews.freebsd.org/D24383

Modified:
  head/sys/kern/kern_jail.c
  head/sys/net/if_ethersubr.c
  head/sys/sys/jail.h

Modified: head/sys/kern/kern_jail.c
==
--- head/sys/kern/kern_jail.c   Sat Apr 18 03:14:16 2020(r360067)
+++ head/sys/kern/kern_jail.c   Sat Apr 18 07:50:30 2020(r360068)
@@ -2920,6 +2920,15 @@ getcredhostid(struct ucred *cred, unsigned long *hosti
mtx_unlock(>cr_prison->pr_mtx);
 }
 
+void
+getjailname(struct ucred *cred, char *name, size_t len)
+{
+
+   mtx_lock(>cr_prison->pr_mtx);
+   strlcpy(name, cred->cr_prison->pr_name, len);
+   mtx_unlock(>cr_prison->pr_mtx);
+}
+
 #ifdef VIMAGE
 /*
  * Determine whether the prison represented by cred owns

Modified: head/sys/net/if_ethersubr.c
==
--- head/sys/net/if_ethersubr.c Sat Apr 18 03:14:16 2020(r360067)
+++ head/sys/net/if_ethersubr.c Sat Apr 18 07:50:30 2020(r360068)
@@ -1419,27 +1419,39 @@ ether_8021q_frame(struct mbuf **mp, struct ifnet *ife,
 
 /*
  * Allocate an address from the FreeBSD Foundation OUI.  This uses a
- * cryptographic hash function on the containing jail's UUID and the interface
- * name to attempt to provide a unique but stable address.  Pseudo-interfaces
- * which require a MAC address should use this function to allocate
- * non-locally-administered addresses.
+ * cryptographic hash function on the containing jail's name, UUID and the
+ * interface name to attempt to provide a unique but stable address.
+ * Pseudo-interfaces which require a MAC address should use this function to
+ * allocate non-locally-administered addresses.
  */
 void
 ether_gen_addr(struct ifnet *ifp, struct ether_addr *hwaddr)
 {
-#defineETHER_GEN_ADDR_BUFSIZ   HOSTUUIDLEN + IFNAMSIZ + 2
SHA1_CTX ctx;
-   char buf[ETHER_GEN_ADDR_BUFSIZ];
+   char *buf;
char uuid[HOSTUUIDLEN + 1];
uint64_t addr;
int i, sz;
char digest[SHA1_RESULTLEN];
+   char jailname[MAXHOSTNAMELEN];
 
getcredhostuuid(curthread->td_ucred, uuid, sizeof(uuid));
-   sz = snprintf(buf, ETHER_GEN_ADDR_BUFSIZ, "%s-%s", uuid, ifp->if_xname);
+   /* If each (vnet) jail would also have a unique hostuuid this would not
+* be necessary. */
+   getjailname(curthread->td_ucred, jailname, sizeof(jailname));
+   sz = asprintf(, M_TEMP, "%s-%s-%s", uuid, if_name(ifp),
+   jailname);
+   if (sz < 0) {
+   /* Fall back to a random mac address. */
+   arc4rand(hwaddr, sizeof(*hwaddr), 0);
+   hwaddr->octet[0] = 0x02;
+   return;
+   }
+
SHA1Init();
SHA1Update(, buf, sz);
SHA1Final(digest, );
+   free(buf, M_TEMP);
 
addr = ((digest[0] << 16) | (digest[1] << 8) | digest[2]) &
OUI_FREEBSD_GENERATED_MASK;

Modified: head/sys/sys/jail.h
==
--- head/sys/sys/jail.h Sat Apr 18 03:14:16 2020(r360067)
+++ head/sys/sys/jail.h Sat Apr 18 07:50:30 2020(r360068)
@@ -382,6 +382,7 @@ void getcredhostname(struct ucred *, char *, size_t);
 void getcreddomainname(struct ucred *, char *, size_t);
 void getcredhostuuid(struct ucred *, char *, size_t);
 void getcredhostid(struct ucred *, unsigned long *);
+void getjailname(struct ucred *cred, char *name, size_t len);
 void prison0_init(void);
 int prison_allow(struct ucred *, unsigned);
 int prison_check(struct ucred *cred1, struct ucred *cred2);
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"