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

[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 <kp at freebsd.org>
> > 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(&cred->cr_prison->pr_mtx);
> > >  }
> > > +void
> > > +getjailname(struct ucred *cred, char *name, size_t len)
> > > +{
> > > +
> > > +	mtx_lock(&cred->cr_prison->pr_mtx);
> > > +	strlcpy(name, cred->cr_prison->pr_name, len);
> > > +	mtx_unlock(&cred->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(&buf, 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
https://git-01.md.hardenedbsd.org/HardenedBSD/pubkeys/src/branch/master/Shawn_Webb/03A4CBEBB82EA5A67D9F3853FF2E67A277F8E1FA.pub.asc
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20200419/e09781cf/attachment.sig>