On Wed, Sep 13, 2017 at 10:23:46AM -0600, Theo de Raadt wrote:
> > On Wed, 13 Sep 2017, Miod Vallat wrote:
> > >
> > > > Forcing uextraloc into .data via
> > > > int uextraloc __attribute__ ((section(".data"))) = 0;
> > > > avoids the crashes.
> > >
> > > Regardless of this bug, the in-tree gcc was modified to default to put
> > > explicitly zero initialized data in .data rather than .bss, i.e. default
> > > to -fno-zero-initialized-in-bss.
> > >
> > > It might be wise to make the default configuration of clang follow this
> > > behaviour as well.
> >
> > Anyone recall the issues that it was originally changed to fix?
> >
> > If it was the kernel config stuff, maybe we should just pass the option
> > explicitly in the kernel Makefiles and not push it on all of userland.
>
> I also don't like it much, because it changes the underlying ABI in such
> a subtle way.
>
> How about initializing the variable to some 'illegal' value,
> recognizing that value when using it, and resetting it to 0 before
> use. Then it doesn't need to rely some crazy ABI tweak.
>
If the attribute to force it into .data isn't wanted the alternative
gets messy as config has a bunch of bogus types and casts and treats the
int values as longs.
Index: mkioconf.c
===================================================================
RCS file: /cvs/src/usr.sbin/config/mkioconf.c,v
retrieving revision 1.36
diff -u -p -r1.36 mkioconf.c
--- mkioconf.c 27 Oct 2016 14:33:30 -0000 1.36
+++ mkioconf.c 14 Sep 2017 02:11:17 -0000
@@ -183,7 +183,7 @@ static long loc[%d] = {", locators.used)
#endif\n\
long extraloc[MAXEXTRALOC] = { -1 };\n\
int nextraloc = MAXEXTRALOC;\n\
-int uextraloc = 0;\n") < 0);
+int uextraloc = -1;\n") < 0);
}
static int nlocnames, maxlocnames = 8;
Index: ukcutil.c
===================================================================
RCS file: /cvs/src/usr.sbin/config/ukcutil.c,v
retrieving revision 1.22
diff -u -p -r1.22 ukcutil.c
--- ukcutil.c 27 Oct 2016 14:32:10 -0000 1.22
+++ ukcutil.c 14 Sep 2017 02:22:33 -0000
@@ -419,6 +419,14 @@ change(int devno)
j = (long *)adjust((caddr_t)nl[I_NEXTRALOC].n_value);
k = (long *)adjust((caddr_t)nl[I_UEXTRALOC].n_value);
+
+ /*
+ * uextraloc is initialised to -1 not 0 so it will be
+ * put in .data instead of .bss so config can modify it
+ */
+ if (*(int *)k == -1)
+ *k = 0;
+
if ((i + *k) > *j) {
printf("Not enough space to change device.\n");
return;
@@ -521,6 +529,14 @@ change_history(int devno, char *str)
j = (long *)adjust((caddr_t)nl[I_NEXTRALOC].n_value);
k = (long *)adjust((caddr_t)nl[I_UEXTRALOC].n_value);
+
+ /*
+ * uextraloc is initialised to -1 not 0 so it will be
+ * put in .data instead of .bss so config can modify it
+ */
+ if (*(int *)k == -1)
+ *k = 0;
+
if ((i + *k) > *j) {
printf("Not enough space to change device.\n");
return;
