Re: FreeBSD Security Advisory FreeBSD-SA-19:23.midi

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: FreeBSD Security Advisory FreeBSD-SA-19:23.midi

Eugene Grosbein-10
21.08.2019 3:12, FreeBSD Security Advisories wrote:

[skip]

> IV.  Workaround
>
> No workaround is available.  Custom kernels without "device sound"
> are not vulnerable.

Is it true that there is no way to disable vulnerable and unneeded device driver
built in GENERIC other that through rebuilding the kernel?

I remember that pre-4.x versions of FreeBSD had visual VGA-based pre-boot configurator
allowing to disable any compiled-in device driver. Don't device.hints(5) or loader(8) have means to do so?

These days GENERIC have LOTS of drivers and it's convenient but unsafe.
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: FreeBSD Security Advisory FreeBSD-SA-19:23.midi

Ian Lepore-3
On Wed, 2019-08-21 at 04:55 +0700, Eugene Grosbein wrote:

> 21.08.2019 3:12, FreeBSD Security Advisories wrote:
>
> [skip]
>
> > IV.  Workaround
> >
> > No workaround is available.  Custom kernels without "device sound"
> > are not vulnerable.
>
> Is it true that there is no way to disable vulnerable and unneeded
> device driver
> built in GENERIC other that through rebuilding the kernel?
>
> I remember that pre-4.x versions of FreeBSD had visual VGA-based pre-
> boot configurator
> allowing to disable any compiled-in device driver. Don't
> device.hints(5) or loader(8) have means to do so?
>
> These days GENERIC have LOTS of drivers and it's convenient but
> unsafe.
>

"No workaround" just seems to be wrong.  Aside from setting the
disabled hint to turn off the driver (or using devctl to turn it off on
a live system), the exploit also requires opening /dev/midistat, so a
viable workaround is to change its permissions so that users can't open
it.

-- Ian

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: FreeBSD Security Advisory FreeBSD-SA-19:23.midi

Mark Johnston-2
On Tue, Aug 20, 2019 at 04:01:39PM -0600, Ian Lepore wrote:

> On Wed, 2019-08-21 at 04:55 +0700, Eugene Grosbein wrote:
> > 21.08.2019 3:12, FreeBSD Security Advisories wrote:
> >
> > [skip]
> >
> > > IV.  Workaround
> > >
> > > No workaround is available.  Custom kernels without "device sound"
> > > are not vulnerable.
> >
> > Is it true that there is no way to disable vulnerable and unneeded
> > device driver
> > built in GENERIC other that through rebuilding the kernel?
> >
> > I remember that pre-4.x versions of FreeBSD had visual VGA-based pre-
> > boot configurator
> > allowing to disable any compiled-in device driver. Don't
> > device.hints(5) or loader(8) have means to do so?
> >
> > These days GENERIC have LOTS of drivers and it's convenient but
> > unsafe.
> >
>
> "No workaround" just seems to be wrong.  Aside from setting the
> disabled hint to turn off the driver (or using devctl to turn it off on
> a live system), the exploit also requires opening /dev/midistat, so a
> viable workaround is to change its permissions so that users can't open
> it.

Yeah, this was an oversight.  The SA text will be amended.
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: FreeBSD Security Advisory FreeBSD-SA-19:23.midi

Bruce Evans-4
In reply to this post by Eugene Grosbein-10
On Wed, 21 Aug 2019, Eugene Grosbein wrote:

> 21.08.2019 3:12, FreeBSD Security Advisories wrote:
>
> [skip]
>
>> IV.  Workaround
>>
>> No workaround is available.  Custom kernels without "device sound"
>> are not vulnerable.
>
> Is it true that there is no way to disable vulnerable and unneeded device driver
> built in GENERIC other that through rebuilding the kernel?
>
> I remember that pre-4.x versions of FreeBSD had visual VGA-based pre-boot configurator

Visual userconfig and command-line userconfug were in all versions of 4.x
too.

> allowing to disable any compiled-in device driver. Don't device.hints(5) or loader(8) have means to do so?

Configuration was unimproved by hints, env and new-bus after 4.x.  In
4.x and earlier, the irq and other parameters, and disable and other
flags, were part of a formal syntax implemented at config(8) time using
yacc and at kernel userconfig time more hackishly and at kernel visual
userconfig time more guishlly.  Now hints and env give a random mostly
undocumented syntax.  Even disable flags don't work right.  New-bus
allows more complicated or just larger topologies which are harder to
control using disable flags.

> These days GENERIC have LOTS of drivers and it's convenient but unsafe.

It is hard to even find the list of (unattached) drivers, or get useful
(fauling) probe messages for drivers that aren't used.

I use the following patch mainly to fix sio and uart probing in uncontrollable
or hard-coded order and/or precendence when both are statically configured.
One must be disabled on a per-device basis, but even disabling doesn't work
without this patch.

The patch preserves some historical mistakes and adds some excessive
verboseness about probe failures.  I'm still waiting for jhb to reply to
mails on 30 Oct 2015 and 23 Jan 2018 asking for a review of this patch
or better a complete fix.

XX Index: subr_bus.c
XX ===================================================================
XX --- subr_bus.c (revision 332488)
XX +++ subr_bus.c (working copy)
XX @@ -2079,6 +2079,12 @@
XX   return (TAILQ_NEXT(last, link));
XX  }
XX
XX +/*
XX + * Keep probing disabled devices for now, in case this has beneficial side
XX + * effects.
XX + */
XX +static volatile int probe_rdisabled = 0;
XX +
XX  /**
XX   * @internal
XX   */
XX @@ -2088,7 +2094,7 @@
XX   devclass_t dc;
XX   driverlink_t best = NULL;
XX   driverlink_t dl;
XX - int result, pri = 0;
XX + int rdisabled, result, unit, pri = 0;
XX   int hasclass = (child->devclass != NULL);
XX
XX   GIANT_REQUIRED;
XX @@ -2139,8 +2145,27 @@
XX   resource_int_value(dl->driver->name, child->unit,
XX      "flags", &child->devflags);
XX
XX - result = DEVICE_PROBE(child);
XX + /* Record other state while the unit is valid. */
XX + unit = child->unit;
XX + rdisabled = resource_disabled(dl->driver->name, unit);
XX
XX + /* See below for more details. */
XX + if (rdisabled) {
XX + device_print_prettyname(dev);
XX + if (probe_rdisabled)
XX + device_printf(child,
XX +    "probing disabled device\n");
XX + else {
XX + device_printf(child,
XX +    "disabled in probe by hints\n");
XX + device_disable(child);
XX + }
XX + }
XX + if (rdisabled && !probe_rdisabled)
XX + result = ENXIO;
XX + else
XX + result = DEVICE_PROBE(child);
XX +
XX   /* Reset flags and devclass before the next probe. */
XX   child->devflags = 0;
XX   if (!hasclass)
XX @@ -2182,6 +2207,30 @@
XX   }
XX
XX   /*
XX + * Ignore the result of probing a disabled device,
XX + * so that disabled devices with higher priorities
XX + * are not preferred, only to do nothing at attach
XX + * time but complete their disablement and fail.
XX + * This is not quite right since it loses the
XX + * accidental (?) feature of being able to disable
XX + * attaching a resource for all drivers by
XX + * disabling it for one driver if there happens to
XX + * one with highest priority (or equal highest,
XX + * with the disabled one preferred because it is
XX + * probed first.
XX + */
XX + if (rdisabled) {
XX + device_print_prettyname(dev);
XX + /* XXX device_printf() fails -- child inval. */
XX + printf("%s%d: disabled in probe by hints\n",
XX +    dl->driver->name, unit);
XX +#ifdef notyet /* XXX don't risk this with invalid child->unit */
XX + device_disable(child);
XX +#endif
XX + continue;
XX + }
XX +
XX + /*
XX   * A priority lower than SUCCESS, remember the
XX   * best matching driver. Initialise the value
XX   * of pri for the first match.

The rest is unrelated (keep the message out of the attach timing).  But
it seems reasonable to get entropy from probe time as well as attach
timing, and that would include timing for spammish probe messages.

XX @@ -2909,8 +2958,6 @@
XX   }
XX
XX   device_sysctl_init(dev);
XX - if (!device_is_quiet(dev))
XX - device_print_child(dev->parent, dev);
XX   attachtime = get_cyclecount();
XX   dev->state = DS_ATTACHING;
XX   if ((error = DEVICE_ATTACH(dev)) != 0) {
XX @@ -2925,6 +2972,8 @@
XX   return (error);
XX   }
XX   attachtime = get_cyclecount() - attachtime;
XX + if (!device_is_quiet(dev))
XX + device_print_child(dev->parent, dev);
XX   /*
XX   * 4 bits per device is a reasonable value for desktop and server
XX   * hardware with good get_cyclecount() implementations, but WILL

Bruce
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: FreeBSD Security Advisory FreeBSD-SA-19:23.midi

Eugene Grosbein-10
30.08.2019 11:03, Bruce Evans wrote:

> The patch preserves some historical mistakes and adds some excessive
> verboseness about probe failures.  I'm still waiting for jhb to reply to
> mails on 30 Oct 2015 and 23 Jan 2018 asking for a review of this patch
> or better a complete fix.

Hmm... Maybe additional notification worth it :-)

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: FreeBSD Security Advisory FreeBSD-SA-19:23.midi

John Baldwin
On 8/30/19 12:56 AM, Eugene Grosbein wrote:
> 30.08.2019 11:03, Bruce Evans wrote:
>
>> The patch preserves some historical mistakes and adds some excessive
>> verboseness about probe failures.  I'm still waiting for jhb to reply to
>> mails on 30 Oct 2015 and 23 Jan 2018 asking for a review of this patch
>> or better a complete fix.
>
> Hmm... Maybe additional notification worth it :-)

Hmm, I've used 'hint.foo.0.disabled=1' with many devices and it works fine.
devctl enable can "undo" the disable post-boot even.

It doesn't disable probing, only attaching once we know which driver a device
is going to use.  As far as I can tell, the patch makes it disable
DEVICE_PROBE as well, but the vast majority of DEVICE_PROBE routines are
idempotent.  Only a handful of legacy ISA drivers use 'return (0)' to try to
pass state from probe to attach via the softc.  Those drivers could choose to
check the disabled flag in their probe routine and/or be fixed to have
idempotent probe routines.  I think the latter is probably the best path
forward.

--
John Baldwin
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: FreeBSD Security Advisory FreeBSD-SA-19:23.midi

Warner Losh
On Fri, Aug 30, 2019 at 10:06 AM John Baldwin <[hidden email]> wrote:

> On 8/30/19 12:56 AM, Eugene Grosbein wrote:
> > 30.08.2019 11:03, Bruce Evans wrote:
> >
> >> The patch preserves some historical mistakes and adds some excessive
> >> verboseness about probe failures.  I'm still waiting for jhb to reply to
> >> mails on 30 Oct 2015 and 23 Jan 2018 asking for a review of this patch
> >> or better a complete fix.
> >
> > Hmm... Maybe additional notification worth it :-)
>
> Hmm, I've used 'hint.foo.0.disabled=1' with many devices and it works fine.
> devctl enable can "undo" the disable post-boot even.
>
> It doesn't disable probing, only attaching once we know which driver a
> device
> is going to use.  As far as I can tell, the patch makes it disable
> DEVICE_PROBE as well, but the vast majority of DEVICE_PROBE routines are
> idempotent.  Only a handful of legacy ISA drivers use 'return (0)' to try
> to
> pass state from probe to attach via the softc.  Those drivers could choose
> to
> check the disabled flag in their probe routine and/or be fixed to have
> idempotent probe routines.  I think the latter is probably the best path
> forward.
>

The problem with disabling before PROBE is that if you have N foo devices,
hint.foo.0.disabled=1 will disable all of them as the probe routines all
return 'not me' and we try foo0 on each new instance... I'm pretty sure
that's why it's not done today and why I didn't do it when I added this
feature because how do you know you have foo0 until probe says 'yup, that's
mine'?

Most of the old ISA routines that returned 0 I think have been deleted.
Maybe all since they were for fussy hardware from before the plug and play
era...

Warner
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: FreeBSD Security Advisory FreeBSD-SA-19:23.midi

Bruce Evans-4
On Fri, 30 Aug 2019, Warner Losh wrote:

> On Fri, Aug 30, 2019 at 10:06 AM John Baldwin <[hidden email]> wrote:
>
>> On 8/30/19 12:56 AM, Eugene Grosbein wrote:
>>> 30.08.2019 11:03, Bruce Evans wrote:
>>>
>>>> The patch preserves some historical mistakes and adds some excessive
>>>> verboseness about probe failures.  I'm still waiting for jhb to reply to
>>>> mails on 30 Oct 2015 and 23 Jan 2018 asking for a review of this patch
>>>> or better a complete fix.
>>>
>>> Hmm... Maybe additional notification worth it :-)
>>
>> Hmm, I've used 'hint.foo.0.disabled=1' with many devices and it works fine.
>> devctl enable can "undo" the disable post-boot even.
>>
>> It doesn't disable probing, only attaching once we know which driver a
>> device
>> is going to use.

It doesn't work fine.  I notice the problem mainly for sio vs uart (I
statically configure lower quality drivers like uart and vt together
with the drivers that I use for regression testing, and select the one
to use by dynamic hints).

sio and uart have hard-coded precedences.  These usually select the
wrong driver when neither is disabled.  I forget if this is a problem
if 1 of the drivers is disabled.  It shouldn't be.

sio's probe is still naughty and depends on setting up some state for sio's
attach (as needed to support old isa irqs not being shareable, but that was
broken before FreeBSD-4).  So its probe is far from idempotent.  uart's
probe is not idempotent either -- see below.

>> As far as I can tell, the patch makes it disable
>> DEVICE_PROBE as well, but the vast majority of DEVICE_PROBE routines are
>> idempotent.

The patch has a flag variable which may be used to keep some of the probing
benahviour.

But the vast majority of probe routines are not idempotent.  They change
the hardware state, not just the softc.  I don't remember any even attempting
to restore the hardware state at the end of probes.  Console drivers give
further complications -- see below.


>> Only a handful of legacy ISA drivers use 'return (0)' to try
>> to
>> pass state from probe to attach via the softc.  Those drivers could choose
>> to
>> check the disabled flag in their probe routine and/or be fixed to have
>> idempotent probe routines.  I think the latter is probably the best path
>> forward.

sio is one of these.

The problem is clearest for console drivers.  Suppose sio and uart are
both statically configured.  One or both must be disabled as a console
driver, else console initialzation of each spamds the one that was
inititalized first.  Normal probe/attach is not involved for this, but
the same disable flag is used for this as fr probe/attach.  This is not
sufficient, due to the probe clobbering the console state, at least
transiently.  In -current:
(1) suppose sio console is initially enabled and uart console is
     iniitially disabled.  Then:
     - sio attaches as console
     - uart probe clobbers sio hardware state transiently and on completion
     - however, sio console doesn't use the ambient sio hardware state --
       it switches the hardware state to a nearly-invariant console state,
       as needed to single-step through higher level state changes in sio
       via tcsetattr(); this works for adverse probes too.
(2) suppose sio console is initially disabled and uart console is
     iniitially endabled.  Then:
     - uart attaches as console
     - sio probe clobbers sio hardware state transiently and on completion
     - uart console does no context switching, so uart crashes even single-
       stepping its own tcsetattr().  It might work accidetally after sio
       probe completes.

> The problem with disabling before PROBE is that if you have N foo devices,
> hint.foo.0.disabled=1 will disable all of them as the probe routines all
> return 'not me' and we try foo0 on each new instance... I'm pretty sure
> that's why it's not done today and why I didn't do it when I added this
> feature because how do you know you have foo0 until probe says 'yup, that's
> mine'?
>
> Most of the old ISA routines that returned 0 I think have been deleted.
> Maybe all since they were for fussy hardware from before the plug and play
> era...

There are still problems with devices being probed and attached on different
buses.  On x86, acpi is probed first and isa last (pnp probes naybe first,
but rarey succeed).  In one of my hardware configurations, IIRC the same
hardware device is attached as uart2 on puc on acpi (or is it pci) and as
sio4 on puc on acpi/pci unless it is fully disabled (in the probe too).

This device works better as memory mapped, but I only committed the i/o
mapped BARs for it in puc.  uart hangs in the probe on the i/o mapped BARs
since the device also needs regshift = 2 and support for regshift != 0 was
axed from puc.  puc has remarkably few memory mapped BARs in it, else it
would hamg more.  sio only supports i/o mapped BARs except in my uncomitted
version.  uart supports memory mapped BARs in puc, but only ones with
regshift=0.  The BAR type is determined dynamically, and the problem is
mostly avoided by omitting memory-mapped BARs in puc data.

Bruce
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"