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 <k...@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
signature.asc
Description: PGP signature
