one XXX in ksh(1)

[Michael W. Bombardieri]

Hello,

In my understanding the reason why texec had to be static was
because the struct member ioact was never initialised.
The call tree is execute() -> comexec() -> exchild() -> execute()
and a fork happens in exchild().
The second execute() dereferences t->ioact on line 115 which causes
SEGV. Setting ioact to NULL explicitly seems better than
doing this via "static".
Does this work for you?

- Michael


Index: exec.c
===
RCS file: /cvs/src/bin/ksh/exec.c,v
retrieving revision 1.68
diff -u -p -u -r1.68 exec.c
--- exec.c  11 Dec 2016 17:49:19 -  1.68
+++ exec.c  18 Dec 2017 07:05:47 -
@@ -409,7 +409,7 @@ comexec(struct op *t, struct tbl *volati
volatile int rv = 0;
char *cp;
char **lastp;
-   static struct op texec; /* Must be static (XXX but why?) */
+   struct op texec;
int type_flags;
int keepasn_ok;
int fcflags = FC_BI|FC_FUNC|FC_PATH;
@@ -688,6 +688,7 @@ comexec(struct op *t, struct tbl *volati
texec.left = t; /* for tprint */
texec.str = tp->val.s;
texec.args = ap;
+   texec.ioact = NULL;
rv = exchild(, flags, xerrok, -1);
break;
}

Re: efiboot: boot i386 kernel (part 1)

[Mike Larkin]

On Sun, Dec 17, 2017 at 12:17:26PM -0800, Mike Larkin wrote:
> On Sun, Dec 17, 2017 at 08:34:52PM +0100, Patrick Wildt wrote:
> > Hi,
> > 
> > I had been looking into booting a 32-bit kernel with efiboot and the
> > first thing I stumbled upon is that when we jump back into 32-bit mode
> > we don't disable the Long Mode Extension.  Thus when the i386 kernel
> > turns on paging, LME goes active as well and "weird things happen".
> > 
> > Not sure this path is worth pursuing, but this diff at least resets
> > EFER correctly.
> > 
> > Opinions? ok?
> > 
> > Patrick
> > 
> > diff --git a/sys/arch/amd64/stand/efiboot/run_i386.S 
> > b/sys/arch/amd64/stand/efiboot/run_i386.S
> > index 1c70f8d4610..63d6f1f1dca 100644
> > --- a/sys/arch/amd64/stand/efiboot/run_i386.S
> > +++ b/sys/arch/amd64/stand/efiboot/run_i386.S
> > @@ -88,6 +88,12 @@ start32a:
> > andl$(~CR4_PAE), %eax
> > movl%eax, %cr4
> >  
> > +   /* Disable LME */
> > +   movl$MSR_EFER, %ecx
> > +   rdmsr
> > +   xor %eax, %eax
> > +   wrmsr
> > +
> > jmp start32b
> >  start32b:
> > .code32
> > 
> 
> Could we ever have the case where EFI set up EFER_NXE? Does that
> need to be preserved (even in i386 mode)?
> 
> -ml
> 

Actually looking a few lines earlier, PAE is being disabled, so perhaps this
is not even a valid question in the first place.

Any reason you're doing the rdmsr there? If you just want to zero it,
xor both the arguments to wrmsr and be done with it, right?

-ml

Re: efiboot: boot i386 kernel (part 1)

[Mike Larkin]

On Sun, Dec 17, 2017 at 08:34:52PM +0100, Patrick Wildt wrote:
> Hi,
> 
> I had been looking into booting a 32-bit kernel with efiboot and the
> first thing I stumbled upon is that when we jump back into 32-bit mode
> we don't disable the Long Mode Extension.  Thus when the i386 kernel
> turns on paging, LME goes active as well and "weird things happen".
> 
> Not sure this path is worth pursuing, but this diff at least resets
> EFER correctly.
> 
> Opinions? ok?
> 
> Patrick
> 
> diff --git a/sys/arch/amd64/stand/efiboot/run_i386.S 
> b/sys/arch/amd64/stand/efiboot/run_i386.S
> index 1c70f8d4610..63d6f1f1dca 100644
> --- a/sys/arch/amd64/stand/efiboot/run_i386.S
> +++ b/sys/arch/amd64/stand/efiboot/run_i386.S
> @@ -88,6 +88,12 @@ start32a:
>   andl$(~CR4_PAE), %eax
>   movl%eax, %cr4
>  
> + /* Disable LME */
> + movl$MSR_EFER, %ecx
> + rdmsr
> + xor %eax, %eax
> + wrmsr
> +
>   jmp start32b
>  start32b:
>   .code32
> 

Could we ever have the case where EFI set up EFER_NXE? Does that
need to be preserved (even in i386 mode)?

-ml

efiboot: boot i386 kernel (part 1)

[Patrick Wildt]

Hi,

I had been looking into booting a 32-bit kernel with efiboot and the
first thing I stumbled upon is that when we jump back into 32-bit mode
we don't disable the Long Mode Extension.  Thus when the i386 kernel
turns on paging, LME goes active as well and "weird things happen".

Not sure this path is worth pursuing, but this diff at least resets
EFER correctly.

Opinions? ok?

Patrick

diff --git a/sys/arch/amd64/stand/efiboot/run_i386.S 
b/sys/arch/amd64/stand/efiboot/run_i386.S
index 1c70f8d4610..63d6f1f1dca 100644
--- a/sys/arch/amd64/stand/efiboot/run_i386.S
+++ b/sys/arch/amd64/stand/efiboot/run_i386.S
@@ -88,6 +88,12 @@ start32a:
andl$(~CR4_PAE), %eax
movl%eax, %cr4
 
+   /* Disable LME */
+   movl$MSR_EFER, %ecx
+   rdmsr
+   xor %eax, %eax
+   wrmsr
+
jmp start32b
 start32b:
.code32

Re: less(1): `!' command

[kshe]

On Sat, 16 Dec 2017 21:52:44 +, Theo de Raadt wrote:
> > On Sat, 16 Dec 2017 19:39:27 +, Theo de Raadt wrote:
> > > > On Sat, 16 Dec 2017 18:13:16 +, Jiri B wrote:
> > > > > On Sat, Dec 16, 2017 at 04:55:44PM +, kshe wrote:
> > > > > > Hi,
> > > > > >
> > > > > > Would a patch to bring back the `!' command to less(1) be accepted? 
> > > > > >  The
> > > > > > commit message for its removal explains that ^Z should be used 
> > > > > > instead,
> > > > > > but that obviously does not work if less(1) is run from something 
> > > > > > else
> > > > > > than an interactive shell, for example when reading manual pages 
> > > > > > from a
> > > > > > vi(1) instance spawned directly by `xterm -e vi' in a window 
> > > > > > manager or
> > > > > > by `neww vi' in a tmux(1) session.
> > > > >
> > > > > Why should less be able to spawn another programs? This would 
> > > > > undermine
> > > > > all pledge work.
> > > >
> > > > Because of at least `v' and `|', less(1) already is able to invoke
> > > > arbitrary programs, and accordingly needs the "proc exec" promise, so
> > > > bringing `!' back would not change anything from a security perspective
> > > > (otherwise, I would obviously not have made such a proposition).
> > > >
> > > > In fact, technically, what I want to do is still currently possible:
> > > > from any less(1) instance, one may use `v' to invoke vi(1), and then use
> > > > vi(1)'s own `!' command as desired.  So the functionality of `!' is
> > > > still there; it was only made more difficult to reach for no apparent
> > > > reason.
> > >
> > > No apparent reason?
> > >
> > > Good you have an opinion.  I have a different opinion: We should look
> > > for rarely used functionality and gut it.
> >
> > I completely agree, and I also completely agree with the rest of what
> > you said.  However, in this particular case, the functionality of `!' is
> > still fully (albeit indirectly) accessible, as shown above, and this is
> > why its deletion, when not immediately followed by that of `|' and `v',
> > made little sense for me.
>
> Oh, so you don't agree.  Or do you.  I can't tell.  You haven't made up
> your mind enough to have a final position?

In the case of less(1), the underlying functionality of `!' (invoking
arbitrary programs) has not been removed at all, as `!' itself was only
one way amongst others of doing that.  Therefore, I would have prefered
that such an endeavour be conducted in steps at least as large as a
pledge(2) category.  You may say this is absolutist, but, in the end,
users might actually be more inclined to accept such removals if they
come with, and thus are justified by, a real and immediate security
benefit, like stricter pledge(2) promises, rather than some vague
theoretical explanation about the global state of their software
environment.

> [...]
>
> > May I go ahead and prepare a patch to remove "proc exec" entirely?
>
> Sure you could try, and see who freaks out.  Exactly what the plan was
> all along.

The minimal diff below does that.  If it is accepted, further cleanups
would need to follow (in particular, removing a few unused variables and
functions), and of course the manual would also need some adjustments.

Index: cmd.h
===
RCS file: /cvs/src/usr.bin/less/cmd.h,v
retrieving revision 1.10
diff -u -p -r1.10 cmd.h
--- cmd.h   6 Nov 2015 15:58:01 -   1.10
+++ cmd.h   17 Dec 2017 12:23:00 -
@@ -42,12 +42,12 @@
 #defineA_FF_LINE   29
 #defineA_BF_LINE   30
 #defineA_VERSION   31
-#defineA_VISUAL32
+/* 32 unused */
 #defineA_F_WINDOW  33
 #defineA_B_WINDOW  34
 #defineA_F_BRACKET 35
 #defineA_B_BRACKET 36
-#defineA_PIPE  37
+/* 37 unused */
 #defineA_INDEX_FILE38
 #defineA_UNDO_SEARCH   39
 #defineA_FF_SCREEN 40
Index: command.c
===
RCS file: /cvs/src/usr.bin/less/command.c,v
retrieving revision 1.31
diff -u -p -r1.31 command.c
--- command.c   12 Jan 2017 20:32:01 -  1.31
+++ command.c   17 Dec 2017 12:23:00 -
@@ -241,12 +241,6 @@ exec_mca(void)
/* If tag structure is loaded then clean it up. */
cleantags();
break;
-   case A_PIPE:
-   if (secure)
-   break;
-   (void) pipe_mark(pipec, cbuf);
-   error("|done", NULL);
-   break;
}
 }
 
@@ -1396,35 +1390,6 @@ again:
c = getcc();
goto again;
 
-   case A_VISUAL:
-   /*
-* Invoke an editor on the input file.
-*/
-   if (secure) {
-

Re: mg: have Insert key toggle overwrite mode by default

[Brian Callahan]

Committed, thanks.

On 12/17/17 05:11, Florian Obser wrote:

OK

On Sat, Dec 16, 2017 at 10:06:59PM +, Lari Rasku wrote:

There's a fairly strong convention among text editors that the Insert
key should toggle overwrite mode.  This is admittedly far more common
among GUI editors, but could mg adopt it as a default anyway?

diff --git usr.bin/mg/ttykbd.c usr.bin/mg/ttykbd.c
index 67bc8e4bd..485291a77 100644
--- usr.bin/mg/ttykbd.c
+++ usr.bin/mg/ttykbd.c
@@ -52,6 +52,8 @@ ttykeymapinit(void)
dobindkey(fundamental_map, "scroll-up", key_npage);
if (key_ppage)
dobindkey(fundamental_map, "scroll-down", key_ppage);
+   if (key_ic)
+   dobindkey(fundamental_map, "overwrite-mode", key_ic);
if (key_dc)
dobindkey(fundamental_map, "delete-char", key_dc);
  

Memory attributes for simplefb userland mappings

[Mark Kettenis]

Running X on my Orange Pi PC2 results in some interesting artifacts
that are clearly cache-related.  That made me realize that we're
mapping the framebuffer as device memory in our kernel and as normal
WB-cachable memory in X.  That is a seriously bad idea.

The diff below switches the userland mappings to non-cachable.  That
is still not perfect as mapping the same memory with different memory
attributes is discouraged.  But at least the architecture treats this
as a seperate case with well-defined semantics.  It certainly is an
improvement over the current situation and removes the artifacts I'm
seeing.

ok?

Ultimately we want to fix this and use the "right" memory attributes
for both the kernel and userland mappings.  I'm not entirely sure yet
what those are.  We certainly don't want WB-cachable, and I don't
think there is a benefit in using WT-cachable as X uses shadowing to
avoid reading from the framebuffer.  If possible, we want to be able
to use "Gathering" which I believe is ARM speak for write-combining.
But whether we need "normal" or "device" memory isn't clear to me.
Currently all framebuffers I've seen are living in normal DRAM.


Index: dev/fdt/simplefb.c
===
RCS file: /cvs/src/sys/dev/fdt/simplefb.c,v
retrieving revision 1.2
diff -u -p -r1.2 simplefb.c
--- dev/fdt/simplefb.c  27 Aug 2017 12:42:22 -  1.2
+++ dev/fdt/simplefb.c  17 Dec 2017 14:04:34 -
@@ -19,6 +19,8 @@
 #include 
 #include 
 
+#include 
+
 #include 
 #include 
 
@@ -276,7 +278,7 @@ simplefb_wsmmap(void *v, off_t off, int 
if (off < 0 || off >= sc->sc_psize)
return -1;
 
-   return sc->sc_paddr + off;
+   return ((sc->sc_paddr + off) | PMAP_NOCACHE);
 }
 
 int

Another regulator framework improvement

[Mark Kettenis]

Diff below allows enabling of more complex regulators.  The "fixed"
regulator support is split off in its own function.  We now respect
the "regulator-allways-on" property by leaving the regulator alone,
assuming it is already turned on.

ok?


Index: dev/ofw/ofw_regulator.c
===
RCS file: /cvs/src/sys/dev/ofw/ofw_regulator.c,v
retrieving revision 1.3
diff -u -p -r1.3 ofw_regulator.c
--- dev/ofw/ofw_regulator.c 16 Dec 2017 21:12:03 -  1.3
+++ dev/ofw/ofw_regulator.c 17 Dec 2017 13:55:57 -
@@ -50,21 +50,13 @@ regulator_register(struct regulator_devi
 }
 
 int
-regulator_set(uint32_t phandle, int enable)
+regulator_fixed_set(int node, int enable)
 {
uint32_t *gpio;
uint32_t startup_delay;
int active;
-   int node;
int len;
 
-   node = OF_getnodebyphandle(phandle);
-   if (node == 0)
-   return -1;
-
-   if (!OF_is_compatible(node, "regulator-fixed"))
-   return -1;
-
pinctrl_byname(node, "default");
 
if (OF_getproplen(node, "enable-active-high") == 0)
@@ -91,6 +83,34 @@ regulator_set(uint32_t phandle, int enab
delay(startup_delay);
 
return 0;
+}
+
+int
+regulator_set(uint32_t phandle, int enable)
+{
+   struct regulator_device *rd;
+   int node;
+
+   node = OF_getnodebyphandle(phandle);
+   if (node == 0)
+   return ENODEV;
+
+   /* Don't mess around with regulators that are always on. */
+   if (OF_getproplen(node, "regulator-always-on") == 0)
+   return 0;
+
+   LIST_FOREACH(rd, _devices, rd_list) {
+   if (rd->rd_phandle == phandle)
+   break;
+   }
+
+   if (rd && rd->rd_enable)
+   return rd->rd_enable(rd->rd_cookie, enable);
+
+   if (OF_is_compatible(node, "regulator-fixed"))
+   return regulator_fixed_set(node, enable);
+
+   return ENODEV;
 }
 
 int
Index: dev/ofw/ofw_regulator.h
===
RCS file: /cvs/src/sys/dev/ofw/ofw_regulator.h,v
retrieving revision 1.4
diff -u -p -r1.4 ofw_regulator.h
--- dev/ofw/ofw_regulator.h 16 Dec 2017 21:12:03 -  1.4
+++ dev/ofw/ofw_regulator.h 17 Dec 2017 13:55:57 -
@@ -23,6 +23,7 @@ struct regulator_device {
void*rd_cookie;
uint32_t (*rd_get_voltage)(void *);
int (*rd_set_voltage)(void *, uint32_t);
+   int (*rd_enable)(void *, int);
 
uint32_t rd_min, rd_max;
 

arm64 wsdisplay options

[Mark Kettenis]

Pretty much a no-brainer.  The raw keyboard stuff is necessary for
running X, and we have VT support in simplefb(4) so let's enable it.

ok?


Index: arch/arm64/conf/GENERIC
===
RCS file: /cvs/src/sys/arch/arm64/conf/GENERIC,v
retrieving revision 1.43
diff -u -p -r1.43 GENERIC
--- arch/arm64/conf/GENERIC 16 Dec 2017 14:15:56 -  1.43
+++ arch/arm64/conf/GENERIC 17 Dec 2017 12:34:43 -
@@ -75,6 +75,10 @@ viomb*   at virtio?
 viornd*at virtio?
 vioscsi*   at virtio?
 
+option WSDISPLAY_COMPAT_USL# VT handling
+option WSDISPLAY_COMPAT_RAWKBD # provide raw scancodes; needed for X11
+option WSDISPLAY_DEFAULTSCREENS=6  # initial number of text consoles
+
 simplefb*  at fdt?
 wsdisplay* at simplefb?
 

Re: mg: have Insert key toggle overwrite mode by default

[Florian Obser]

OK

On Sat, Dec 16, 2017 at 10:06:59PM +, Lari Rasku wrote:
> There's a fairly strong convention among text editors that the Insert
> key should toggle overwrite mode.  This is admittedly far more common
> among GUI editors, but could mg adopt it as a default anyway?
> 
> diff --git usr.bin/mg/ttykbd.c usr.bin/mg/ttykbd.c
> index 67bc8e4bd..485291a77 100644
> --- usr.bin/mg/ttykbd.c
> +++ usr.bin/mg/ttykbd.c
> @@ -52,6 +52,8 @@ ttykeymapinit(void)
>   dobindkey(fundamental_map, "scroll-up", key_npage);
>   if (key_ppage)
>   dobindkey(fundamental_map, "scroll-down", key_ppage);
> + if (key_ic)
> + dobindkey(fundamental_map, "overwrite-mode", key_ic);
>   if (key_dc)
>   dobindkey(fundamental_map, "delete-char", key_dc);
>  
> 

-- 
I'm not entirely sure you are real.

csh dounsetenv()

[Michael W. Bombardieri]

Hello,

The free() at the top of dounsetenv() in csh(1) isn't needed
because name is always freed before returning at bottom of function.
Also, name itself is never returned so it doesn't need to be static.

./csh
setenv HEY YU
unsetenv HEY
printenv

I ran the above and it seems to work the same as before.

- Michael


Index: func.c
===
RCS file: /cvs/src/bin/csh/func.c,v
retrieving revision 1.36
diff -u -p -u -r1.36 func.c
--- func.c  16 Dec 2017 10:27:21 -  1.36
+++ func.c  17 Dec 2017 09:41:01 -
@@ -924,11 +924,9 @@ void
 /*ARGSUSED*/
 dounsetenv(Char **v, struct command *t)
 {
-Char  **ep, *p, *n;
+Char  **ep, *p, *n, *name;
 int i, maxi;
-static Char *name = NULL;
 
-free(name);
 /*
  * Find the longest environment variable
  */

Re: ypldap patch 5: fix aldap_close

[Vadim Zhukov]

2017-12-17 6:35 GMT+03:00 Jonathan Matthew :
> On Sat, Dec 16, 2017 at 08:38:59PM +0300, Vadim Zhukov wrote:
>> 2017-12-06 19:12 GMT+03:00 Vadim Zhukov :
>> >> The aldap_close() return value is never checked, and I do not see
>> >> any good reason to do that. Also, in case close(2) fails, it'll miss
>> >> freeing other data.
>> >
>> > I'm blind. :-\
>> >
>> > The problem I was looking for was right here: the aldap_close() closes
>> > the wrong file descriptor. So here is a better patch that solves
>> > actual leak. I ever treat this as a candidate for -STABLE, since
>> > when ypldap get stuck, you could be locked out of system.
>>
>> Sorry for noise, I'm just trying to make this patch go in. I think it
>> should because it fixes a real issue seen in the wild (if an isolated
>> AD-enabled LAN could be called "wild"). Well, actually it fixes two
>> issues, but I found zero code paths for making close() fail in current
>> code.
>>
>> The patched version happily runs for more than a week on (otherwise) 
>> 6.2-STABLE.
>
> Your diff is correct, but only for the non-tls case.  I missed cleaning
> up the tls context when I added tls support, and we need to keep the fd
> around so we can close it, since tls_close doesn't close file descriptors
> that libtls didn't open.
>
> ok?
>
> Index: aldap.c
> ===
> RCS file: /cvs/src/usr.sbin/ypldap/aldap.c,v
> retrieving revision 1.37
> diff -u -p -u -p -r1.37 aldap.c
> --- aldap.c 30 May 2017 09:33:31 -  1.37
> +++ aldap.c 17 Dec 2017 03:19:02 -
> @@ -70,10 +70,11 @@ aldap_application(struct ber_element *el
>  int
>  aldap_close(struct aldap *al)
>  {
> -   if (al->fd != -1)
> -   if (close(al->ber.fd) == -1)
> -   return (-1);
> -
> +   if (al->tls != NULL) {
> +   tls_close(al->tls);
> +   tls_free(al->tls);
> +   }
> +   close(al->fd);
> ber_free(>ber);
> evbuffer_free(al->buf);
> free(al);
> @@ -120,7 +121,6 @@ aldap_tls(struct aldap *ldap, struct tls
> return (-1);
> }
>
> -   ldap->fd = -1;
> return (0);
>  }

Thank you for answering.

The diff reads correct to me, okay zhuk@.

--
  WBR,
  Vadim Zhukov