Re: [PATCH v5 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[One Thousand Gnomes]

> > Which is the sudo case and why sudo uses a separate pty/tty pair as it's
> > not just TIOCSTI that's an issue but there are a load of ioctls that do
> > things like cause signals to the process or are just annoying -
> > vhangup(), changing the speed etc
> >
> > (And for console changing the keymap - which is a nasty one)
> >  
> 
> Are any of these annoyances potential security issues? I would be happy
> to add patches or modify this one to include extra hardening measures.

Or you could just use pty/tty pairs properly the way sudo and other
applications do perfectly well.

Lots of them are potential security issues - if I sent your console to
1x1 char, change the font and keymap you'd proably be peeved 8-)

It's not about hardening against all these (which would break lots of
legitimate use cases), it's about having the affected applications do the
right thing.

It makes sense that TIOCSTI honours namespaces. However it and everything
else are correctly handled by creating the lower security level process
with its own pty/tty pair.

Alan

Re: [PATCH v5 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[One Thousand Gnomes]

> > Which is the sudo case and why sudo uses a separate pty/tty pair as it's
> > not just TIOCSTI that's an issue but there are a load of ioctls that do
> > things like cause signals to the process or are just annoying -
> > vhangup(), changing the speed etc
> >
> > (And for console changing the keymap - which is a nasty one)
> >  
> 
> Are any of these annoyances potential security issues? I would be happy
> to add patches or modify this one to include extra hardening measures.

Or you could just use pty/tty pairs properly the way sudo and other
applications do perfectly well.

Lots of them are potential security issues - if I sent your console to
1x1 char, change the font and keymap you'd proably be peeved 8-)

It's not about hardening against all these (which would break lots of
legitimate use cases), it's about having the affected applications do the
right thing.

It makes sense that TIOCSTI honours namespaces. However it and everything
else are correctly handled by creating the lower security level process
with its own pty/tty pair.

Alan

Re: [PATCH v5 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[One Thousand Gnomes]

> open() what? As far as I know, for System-V PTYs, there is no path you can
> open() that will give you the PTY master. Am I missing something?

Sorry brain fade - no.
> 
> >> > If I want to do the equvalent of the TIOCSTI attack then I fork a process
> >> > and exit the parent. The child can now use ptrace to reprogram your shell
> >> > to do whatever interesting things it likes (eg running child processes
> >> > called "su" via a second pty/tty pair). Not exactly rocket science.  
> >>
> >> Why would the child be able to ptrace the shell? AFAICS, in the most
> >> relevant scenarios, the child can't ptrace the shell because the
> >> shell has a different UID (in the case of e.g. su or sudo). In other  
> >
> > If I am the attacker wanting to type something into your su when you go
> > and su from my account, or where the user account is trojanned I do the
> > following
> >
> > fork
> > exit parent
> > child ptraces the shell (same uid as it's not setuid)
> >
> > You type "su" return
> > The modified shell opens a new pty/tty pair and runs su over it
> > My ptrace hooks watch the pty/tty traffic until you go to the loo
> > My ptrace hooks switch the console
> > My ptrace hooks type lots of stuff and hack your machine while eating the
> > output
> >
> > and you come back, do stuff and then exit
> >
> > And if you are in X it's even easier and I don't even need to care about
> > sessions or anything. X has no mechanism to sanely fix the problem, but
> > Wayland does.  
> 
> I think the "When using a program like su or sudo" in the patch description
> refers to the usecase where you go from a more privileged context (e.g. a
> root shell) to a less privileged one (e.g. a shell as a service-specific
> account used to run a daemon), not the other way around.

Which is the sudo case and why sudo uses a separate pty/tty pair as it's
not just TIOCSTI that's an issue but there are a load of ioctls that do
things like cause signals to the process or are just annoying -
vhangup(), changing the speed etc

(And for console changing the keymap - which is a nasty one)

> [However, I do think that it's a nice side effect of this patch that it will
> prevent a malicious program from directly injecting something like an
> SSH command into my shell in a sufficiently hardened environment
> (with LSM restrictions that prevent the malicious program from opening
> SSH keyfiles or executing another program that can do that). Although
> you could argue that in such a case, the LSM should be taking care of
> blocking TIOCSTI.]

I would submit that creating a new pty/tty pair is the proper answer for
that case however. Making the tty calls respect namespaces is however
still a no-brainer IMHO.

Alan

Re: [PATCH v5 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[One Thousand Gnomes]

> open() what? As far as I know, for System-V PTYs, there is no path you can
> open() that will give you the PTY master. Am I missing something?

Sorry brain fade - no.
> 
> >> > If I want to do the equvalent of the TIOCSTI attack then I fork a process
> >> > and exit the parent. The child can now use ptrace to reprogram your shell
> >> > to do whatever interesting things it likes (eg running child processes
> >> > called "su" via a second pty/tty pair). Not exactly rocket science.  
> >>
> >> Why would the child be able to ptrace the shell? AFAICS, in the most
> >> relevant scenarios, the child can't ptrace the shell because the
> >> shell has a different UID (in the case of e.g. su or sudo). In other  
> >
> > If I am the attacker wanting to type something into your su when you go
> > and su from my account, or where the user account is trojanned I do the
> > following
> >
> > fork
> > exit parent
> > child ptraces the shell (same uid as it's not setuid)
> >
> > You type "su" return
> > The modified shell opens a new pty/tty pair and runs su over it
> > My ptrace hooks watch the pty/tty traffic until you go to the loo
> > My ptrace hooks switch the console
> > My ptrace hooks type lots of stuff and hack your machine while eating the
> > output
> >
> > and you come back, do stuff and then exit
> >
> > And if you are in X it's even easier and I don't even need to care about
> > sessions or anything. X has no mechanism to sanely fix the problem, but
> > Wayland does.  
> 
> I think the "When using a program like su or sudo" in the patch description
> refers to the usecase where you go from a more privileged context (e.g. a
> root shell) to a less privileged one (e.g. a shell as a service-specific
> account used to run a daemon), not the other way around.

Which is the sudo case and why sudo uses a separate pty/tty pair as it's
not just TIOCSTI that's an issue but there are a load of ioctls that do
things like cause signals to the process or are just annoying -
vhangup(), changing the speed etc

(And for console changing the keymap - which is a nasty one)

> [However, I do think that it's a nice side effect of this patch that it will
> prevent a malicious program from directly injecting something like an
> SSH command into my shell in a sufficiently hardened environment
> (with LSM restrictions that prevent the malicious program from opening
> SSH keyfiles or executing another program that can do that). Although
> you could argue that in such a case, the LSM should be taking care of
> blocking TIOCSTI.]

I would submit that creating a new pty/tty pair is the proper answer for
that case however. Making the tty calls respect namespaces is however
still a no-brainer IMHO.

Alan

Re: [PATCH v5 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[One Thousand Gnomes]

> Really? By "pty", are you referring to the master? If so, as far as I know,
> to go from the slave to the master, you need one of:
> 
>  - ptrace access to a process that already has an FD to the master, via
>ptrace() or so (/proc/$pid/fd/$fd won't work)
>  - for a BSD PTY (which AFAIK isn't used much anymore), access to
>/dev/ptyXX

fstat() and then open *assuming* I have permissions.

> 
> Am I overlooking something?
> 
> > If I want to do the equvalent of the TIOCSTI attack then I fork a process
> > and exit the parent. The child can now use ptrace to reprogram your shell
> > to do whatever interesting things it likes (eg running child processes
> > called "su" via a second pty/tty pair). Not exactly rocket science.  
> 
> Why would the child be able to ptrace the shell? AFAICS, in the most
> relevant scenarios, the child can't ptrace the shell because the
> shell has a different UID (in the case of e.g. su or sudo). In other

If I am the attacker wanting to type something into your su when you go
and su from my account, or where the user account is trojanned I do the
following

fork
exit parent
child ptraces the shell (same uid as it's not setuid)

You type "su" return
The modified shell opens a new pty/tty pair and runs su over it
My ptrace hooks watch the pty/tty traffic until you go to the loo
My ptrace hooks switch the console 
My ptrace hooks type lots of stuff and hack your machine while eating the
output

and you come back, do stuff and then exit

And if you are in X it's even easier and I don't even need to care about
sessions or anything. X has no mechanism to sanely fix the problem, but
Wayland does.

Fortunately in any sane container case we don't give X11 handles to the
container contents. In insane cases (flatpak for example) you lose.

> scenarios, Yama, if enabled, should still stop you from doing that.
> And even with containers that have the same UID as the calling user,
> which I think is not exactly a good approach from a security perspective,
> the PID namespace should stop you from ptracing things outside the
> container.

For the case where the goal is to stop something leaking out of a
container then I agree entirely - namespaces can be used to play
whackamole with that particular one or you could use a pty/tty pair which
is how "sudo" solves the same problem space.

Disabling TIOCSTI via some magic Kconfig is silly, but making it
namespace hard is not.

If you really care about container security you could just a lightweight
vm instead but that's a different discussion ;-)

Alan

Re: [PATCH v5 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[One Thousand Gnomes]

> Really? By "pty", are you referring to the master? If so, as far as I know,
> to go from the slave to the master, you need one of:
> 
>  - ptrace access to a process that already has an FD to the master, via
>ptrace() or so (/proc/$pid/fd/$fd won't work)
>  - for a BSD PTY (which AFAIK isn't used much anymore), access to
>/dev/ptyXX

fstat() and then open *assuming* I have permissions.

> 
> Am I overlooking something?
> 
> > If I want to do the equvalent of the TIOCSTI attack then I fork a process
> > and exit the parent. The child can now use ptrace to reprogram your shell
> > to do whatever interesting things it likes (eg running child processes
> > called "su" via a second pty/tty pair). Not exactly rocket science.  
> 
> Why would the child be able to ptrace the shell? AFAICS, in the most
> relevant scenarios, the child can't ptrace the shell because the
> shell has a different UID (in the case of e.g. su or sudo). In other

If I am the attacker wanting to type something into your su when you go
and su from my account, or where the user account is trojanned I do the
following

fork
exit parent
child ptraces the shell (same uid as it's not setuid)

You type "su" return
The modified shell opens a new pty/tty pair and runs su over it
My ptrace hooks watch the pty/tty traffic until you go to the loo
My ptrace hooks switch the console 
My ptrace hooks type lots of stuff and hack your machine while eating the
output

and you come back, do stuff and then exit

And if you are in X it's even easier and I don't even need to care about
sessions or anything. X has no mechanism to sanely fix the problem, but
Wayland does.

Fortunately in any sane container case we don't give X11 handles to the
container contents. In insane cases (flatpak for example) you lose.

> scenarios, Yama, if enabled, should still stop you from doing that.
> And even with containers that have the same UID as the calling user,
> which I think is not exactly a good approach from a security perspective,
> the PID namespace should stop you from ptracing things outside the
> container.

For the case where the goal is to stop something leaking out of a
container then I agree entirely - namespaces can be used to play
whackamole with that particular one or you could use a pty/tty pair which
is how "sudo" solves the same problem space.

Disabling TIOCSTI via some magic Kconfig is silly, but making it
namespace hard is not.

If you really care about container security you could just a lightweight
vm instead but that's a different discussion ;-)

Alan

Re: [PATCH 00/16] Intel FPGA Device Drivers

[One Thousand Gnomes]

> That is where we disagree. I do not see bitstream as firmware. For instance
> now you can run OpenCL on some FPGA, so this is exactly like GPU we should
> request open source stack from OpenCL down to bitstream.

It's an accelerator with a bunch of firmwares where you load the right
one. We've got lots of those in Linux already. Your GPU probably needs
firmware as well in just the same way.

> For me this is not enough (tool to load bitstream).

Unfortunately that isn't likely to change for any major FPGA device in
the near future. If you could load arbitrary bit patterns into an FPGA
then in most cases that also means you could physically destroy the
hardware.

Alan

Re: [PATCH 00/16] Intel FPGA Device Drivers

[One Thousand Gnomes]

> That is where we disagree. I do not see bitstream as firmware. For instance
> now you can run OpenCL on some FPGA, so this is exactly like GPU we should
> request open source stack from OpenCL down to bitstream.

It's an accelerator with a bunch of firmwares where you load the right
one. We've got lots of those in Linux already. Your GPU probably needs
firmware as well in just the same way.

> For me this is not enough (tool to load bitstream).

Unfortunately that isn't likely to change for any major FPGA device in
the near future. If you could load arbitrary bit patterns into an FPGA
then in most cases that also means you could physically destroy the
hardware.

Alan

Re: [PATCH v5 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[One Thousand Gnomes]

On Tue, 25 Apr 2017 15:56:32 +0200
Jann Horn  wrote:

> On Tue, Apr 25, 2017 at 3:47 PM, Alan Cox  wrote:
> >> There could be a few user programs that would be effected by this
> >> change.
> >> See: 
> >> notable programs are: agetty, csh, xemacs and tcsh
> >>
> >> However, I still believe that this change is worth it given that the
> >> Kconfig defaults to n. This will be a feature that is turned on for the
> >> same reason that people activate it when using grsecurity. Users of this
> >> opt-in feature will realize that they are choosing security over some OS
> >> features  
> >
> > Only in this case they are not.
> >
> > If I am at the point I have the ability to send you TIOCSTI you already
> > lost because I can just open /dev/tty to get access to my controlling tty
> > and use write().  
> 
> In terms of PTYs, this patch does not try to prevent writes to a slave
> device (which afaik is what /dev/tty will give you). It tries to prevent the
> equivalent of writes to the master device. As far as I know, there is no
> way to go from a slave to the corresponding master without having
> access to the master in some other way already.

Ok so the point I was trying to make about write and read is I can
already trash your channel when you su. Probably less irritatingly.

In the pty case yes I can go from the tty to pty trivially and then open
it, however the owner of the pty side would normally have exclusivity so
while it's a potential hole it isn't a trivial one.

If I want to do the equvalent of the TIOCSTI attack then I fork a process
and exit the parent. The child can now use ptrace to reprogram your shell
to do whatever interesting things it likes (eg running child processes
called "su" via a second pty/tty pair). Not exactly rocket science.

The tty layer does not try to manage this because it can't and two
processes with the same uid are not protected from one another in the
traditional Unix model. As with anything else when you try and glue
namespaces on top of a model not designed for it you get a pile of holes.

There is no safe way to fix it if you can't trust the environment you are
communicating through. Secure practice is either to make another
connection or if local to switch console and use SAK then login.

In the namespaces case it certainly makes sense to forbid a process in
one namespace from typing characters into another namespace but to me
that implies that tty sessions/job control are namespaced, and that makes
transitioning namespace or even just typing stuff into a docker container
shell rather more tricky to get right if you have to be in the right tty
session _and_ namespace to use the tty.

Alan

Re: [PATCH v5 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[One Thousand Gnomes]

On Tue, 25 Apr 2017 15:56:32 +0200
Jann Horn  wrote:

> On Tue, Apr 25, 2017 at 3:47 PM, Alan Cox  wrote:
> >> There could be a few user programs that would be effected by this
> >> change.
> >> See: 
> >> notable programs are: agetty, csh, xemacs and tcsh
> >>
> >> However, I still believe that this change is worth it given that the
> >> Kconfig defaults to n. This will be a feature that is turned on for the
> >> same reason that people activate it when using grsecurity. Users of this
> >> opt-in feature will realize that they are choosing security over some OS
> >> features  
> >
> > Only in this case they are not.
> >
> > If I am at the point I have the ability to send you TIOCSTI you already
> > lost because I can just open /dev/tty to get access to my controlling tty
> > and use write().  
> 
> In terms of PTYs, this patch does not try to prevent writes to a slave
> device (which afaik is what /dev/tty will give you). It tries to prevent the
> equivalent of writes to the master device. As far as I know, there is no
> way to go from a slave to the corresponding master without having
> access to the master in some other way already.

Ok so the point I was trying to make about write and read is I can
already trash your channel when you su. Probably less irritatingly.

In the pty case yes I can go from the tty to pty trivially and then open
it, however the owner of the pty side would normally have exclusivity so
while it's a potential hole it isn't a trivial one.

If I want to do the equvalent of the TIOCSTI attack then I fork a process
and exit the parent. The child can now use ptrace to reprogram your shell
to do whatever interesting things it likes (eg running child processes
called "su" via a second pty/tty pair). Not exactly rocket science.

The tty layer does not try to manage this because it can't and two
processes with the same uid are not protected from one another in the
traditional Unix model. As with anything else when you try and glue
namespaces on top of a model not designed for it you get a pile of holes.

There is no safe way to fix it if you can't trust the environment you are
communicating through. Secure practice is either to make another
connection or if local to switch console and use SAK then login.

In the namespaces case it certainly makes sense to forbid a process in
one namespace from typing characters into another namespace but to me
that implies that tty sessions/job control are namespaced, and that makes
transitioning namespace or even just typing stuff into a docker container
shell rather more tricky to get right if you have to be in the right tty
session _and_ namespace to use the tty.

Alan

Re: [PATCH v2 2/9] tty_port: allow a port to be opened with a tty that has no file handle

[One Thousand Gnomes]

On Mon, 16 Jan 2017 16:54:29 -0600
Rob Herring  wrote:

> From: Alan Cox 
> 
> Let us create tty objects entirely in kernel space. Untested proposal to
> show why all the ideas around rewriting half the uart stack are not needed.
> 
> With this a kernel created non file backed tty object could be used to handle
> data, and set terminal modes. Not all ldiscs can cope with this as N_TTY in
> particular has to work back to the fs/tty layer.
> 
> The tty_port code is however otherwise clean of file handles as far as I can
> tell as is the low level tty port write path used by the ldisc, the
> configuration low level interfaces and most of the ldiscs.
> 
> Currently you don't have any exposure to see tty hangups because those are
> built around the file layer. However a) it's a fixed port so you probably
> don't care about that b) if you do we can add a callback and c) you almost
> certainly don't want the userspace tear down/rebuild behaviour anyway.
> 
> This should however be sufficient if we wanted for example to enumerate all
> the bluetooth bound fixed ports via ACPI and make them directly available.
> It doesn't deal with the case of a user opening a port that's also kernel
> opened and that would need some locking out (so it returned EBUSY if bound
> to a kernel device of some kind). That needs resolving along with how you
> "up" or "down" your new bluetooth device, or enumerate it while providing
> the existing tty API to avoid regressions (and to debug).
> 
> Alan
> Reviewed-by: Andy Shevchenko 
> Reviewed-By: Sebastian Reichel 
> ---
> Alan, Need your SoB here.

Signed-off-by: Alan Cox 

Alan

Re: [PATCH v2 2/9] tty_port: allow a port to be opened with a tty that has no file handle

[One Thousand Gnomes]

On Mon, 16 Jan 2017 16:54:29 -0600
Rob Herring  wrote:

> From: Alan Cox 
> 
> Let us create tty objects entirely in kernel space. Untested proposal to
> show why all the ideas around rewriting half the uart stack are not needed.
> 
> With this a kernel created non file backed tty object could be used to handle
> data, and set terminal modes. Not all ldiscs can cope with this as N_TTY in
> particular has to work back to the fs/tty layer.
> 
> The tty_port code is however otherwise clean of file handles as far as I can
> tell as is the low level tty port write path used by the ldisc, the
> configuration low level interfaces and most of the ldiscs.
> 
> Currently you don't have any exposure to see tty hangups because those are
> built around the file layer. However a) it's a fixed port so you probably
> don't care about that b) if you do we can add a callback and c) you almost
> certainly don't want the userspace tear down/rebuild behaviour anyway.
> 
> This should however be sufficient if we wanted for example to enumerate all
> the bluetooth bound fixed ports via ACPI and make them directly available.
> It doesn't deal with the case of a user opening a port that's also kernel
> opened and that would need some locking out (so it returned EBUSY if bound
> to a kernel device of some kind). That needs resolving along with how you
> "up" or "down" your new bluetooth device, or enumerate it while providing
> the existing tty API to avoid regressions (and to debug).
> 
> Alan
> Reviewed-by: Andy Shevchenko 
> Reviewed-By: Sebastian Reichel 
> ---
> Alan, Need your SoB here.

Signed-off-by: Alan Cox 

Alan

Re: [PATCH 6/9] dt/bindings: Add a serial/UART attached device binding

[One Thousand Gnomes]

On Fri,  6 Jan 2017 10:26:32 -0600
Rob Herring  wrote:

> Add a common binding for describing serial/UART attached devices. Common
> examples are Bluetooth, WiFi, NFC and GPS devices.
> 
> Serial attached devices are represented as child nodes of a UART node.
> This may need to be extended for more complex devices with multiple
> interfaces, but for the simple cases a child node is sufficient.
> 
> Signed-off-by: Rob Herring 
> ---
>  .../devicetree/bindings/serial/slave-device.txt| 34 
> ++
>  1 file changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/slave-device.txt
> 
> diff --git a/Documentation/devicetree/bindings/serial/slave-device.txt 
> b/Documentation/devicetree/bindings/serial/slave-device.txt
> new file mode 100644
> index ..9b7c2d651345
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/slave-device.txt
> @@ -0,0 +1,34 @@
> +Serial Slave Device DT binding
> +
> +This documents the binding structure and common properties for serial
> +attached devices. Common examples include Bluetooth, WiFi, NFC and GPS
> +devices.
> +
> +qSerial

Stray 'q' ??

Re: [PATCH 6/9] dt/bindings: Add a serial/UART attached device binding

[One Thousand Gnomes]

On Fri,  6 Jan 2017 10:26:32 -0600
Rob Herring  wrote:

> Add a common binding for describing serial/UART attached devices. Common
> examples are Bluetooth, WiFi, NFC and GPS devices.
> 
> Serial attached devices are represented as child nodes of a UART node.
> This may need to be extended for more complex devices with multiple
> interfaces, but for the simple cases a child node is sufficient.
> 
> Signed-off-by: Rob Herring 
> ---
>  .../devicetree/bindings/serial/slave-device.txt| 34 
> ++
>  1 file changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/slave-device.txt
> 
> diff --git a/Documentation/devicetree/bindings/serial/slave-device.txt 
> b/Documentation/devicetree/bindings/serial/slave-device.txt
> new file mode 100644
> index ..9b7c2d651345
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/slave-device.txt
> @@ -0,0 +1,34 @@
> +Serial Slave Device DT binding
> +
> +This documents the binding structure and common properties for serial
> +attached devices. Common examples include Bluetooth, WiFi, NFC and GPS
> +devices.
> +
> +qSerial

Stray 'q' ??

Re: [PATCH] serial/8250: remove comment about schedule_timeout

[One Thousand Gnomes]

On Thu,  5 Jan 2017 13:48:40 -0200
Thadeu Lima de Souza Cascardo  wrote:

> Ted T'so has added the function size_fifo in 1999 for the 2.3 series
> [1], a long time ago.
> 
> During the 2.5 cycle, Russell King has restructured the serial drivers
> and, in that process, has suggested using schedule_timeout instead of
> mdelay in size_fifo. [2]
> 
> It was only at 2.6.7 that Greg Kroah-Hartman added the msleep function
> to the core kernel, as people were starting to duplicate it. [3]
> 
> However, as size_fifo is called under a spinlock from the autoconfig
> function, we might not use msleep here, so removing that comment is the
> appropriate thing to do.

No.. it's still a flaw in the driver that we can't use msleep here. It's
one of those things that people want to know if the probe locking ever
gets restructured.

Alan

Re: [PATCH] serial/8250: remove comment about schedule_timeout

[One Thousand Gnomes]

On Thu,  5 Jan 2017 13:48:40 -0200
Thadeu Lima de Souza Cascardo  wrote:

> Ted T'so has added the function size_fifo in 1999 for the 2.3 series
> [1], a long time ago.
> 
> During the 2.5 cycle, Russell King has restructured the serial drivers
> and, in that process, has suggested using schedule_timeout instead of
> mdelay in size_fifo. [2]
> 
> It was only at 2.6.7 that Greg Kroah-Hartman added the msleep function
> to the core kernel, as people were starting to duplicate it. [3]
> 
> However, as size_fifo is called under a spinlock from the autoconfig
> function, we might not use msleep here, so removing that comment is the
> appropriate thing to do.

No.. it's still a flaw in the driver that we can't use msleep here. It's
one of those things that people want to know if the probe locking ever
gets restructured.

Alan

Re: Geode LX AES/RNG driver triggers warning

[One Thousand Gnomes]

On Sat, 31 Dec 2016 00:58:54 +0100
David Gstir  wrote:

> Hi!
> 
> I recently tested kernel v4.9 on my AMD Geode platform and noticed that its 
> AES hardware driver triggers this warning on initialization:

...

> I narrowed it down to commit 6e9b5e76882c ("hwrng: geode - Migrate to managed 
> API") which seems to introduce this. It looks to me like some issue between 
> devres, the Geode hwrng and AES drivers which both use the same PCI device.

It does

> I'm no expert here, but I curious if this will cause any issues when using 
> the hardware crypto drivers and also what's the best way to get rid of this?

Probably to create an mfd device that turns the PCI device into two MFD
devices and bind AES and hwrng one to each MFD device. Take a look in
drivers/mfd. That would also fix the uglies in mod_init for the rng
driver.

Alan

Re: Geode LX AES/RNG driver triggers warning

[One Thousand Gnomes]

On Sat, 31 Dec 2016 00:58:54 +0100
David Gstir  wrote:

> Hi!
> 
> I recently tested kernel v4.9 on my AMD Geode platform and noticed that its 
> AES hardware driver triggers this warning on initialization:

...

> I narrowed it down to commit 6e9b5e76882c ("hwrng: geode - Migrate to managed 
> API") which seems to introduce this. It looks to me like some issue between 
> devres, the Geode hwrng and AES drivers which both use the same PCI device.

It does

> I'm no expert here, but I curious if this will cause any issues when using 
> the hardware crypto drivers and also what's the best way to get rid of this?

Probably to create an mfd device that turns the PCI device into two MFD
devices and bind AES and hwrng one to each MFD device. Take a look in
drivers/mfd. That would also fix the uglies in mod_init for the rng
driver.

Alan

Re: [PATCH 1/4] serial: core: Add LED trigger support

[One Thousand Gnomes]

> > If we have different UART drivers, only one of them provides ttyS*, no?
> > Other drivers will have to use another namespace.  
> 
> I remember this was a problem a couple of years ago last I tried, with
> the 8250 driver being actually preventing other drivers from using
> ttyS*, but if you don't have 8250 taking over the low ttyS numbers, you
> may run into a case where several drivers successfully register their
> character devices under a ttyS* numbering scheme.
> 
> Whether this is hypothetical or not nowadays, it may be nicer to provide
> a more uniquely distinct name like what dev_name() returns, or
> ttyS*-driver-tx for instance?

It needs to be at the tty layer anyway or you will break low end
machines. On a low end system with a 16450A UART you've got something
like 160 microseconds at 57600 baud before you must have exited the
IRQ handler, re-entered it and be reading the data register again. The
current proposed patches touching the 8250 IRQ path are almost certainly
going to cause regressions on old machines.

So you need to hook receive at a point outside of the IRQ layer (eg when
the workqueue starts feeding it into the ldisc), but on the tx side you
need to hook the write method of the tty really, because tty_write sends
data to the ldisc and what happens then is up to the ldisc.

Even there though you have cases that are terribly latency dependant like
some of the dumb programming tools that don't window. If you halve
someones programming rate for microcontrollers you are going to get hate
mail 8) For RX that can mostly be avoided by queuing the data then doing
the LEDs, for TX I'm not clear you can easily hide the latency.

Alan

Re: [PATCH 1/4] serial: core: Add LED trigger support

[One Thousand Gnomes]

> > If we have different UART drivers, only one of them provides ttyS*, no?
> > Other drivers will have to use another namespace.  
> 
> I remember this was a problem a couple of years ago last I tried, with
> the 8250 driver being actually preventing other drivers from using
> ttyS*, but if you don't have 8250 taking over the low ttyS numbers, you
> may run into a case where several drivers successfully register their
> character devices under a ttyS* numbering scheme.
> 
> Whether this is hypothetical or not nowadays, it may be nicer to provide
> a more uniquely distinct name like what dev_name() returns, or
> ttyS*-driver-tx for instance?

It needs to be at the tty layer anyway or you will break low end
machines. On a low end system with a 16450A UART you've got something
like 160 microseconds at 57600 baud before you must have exited the
IRQ handler, re-entered it and be reading the data register again. The
current proposed patches touching the 8250 IRQ path are almost certainly
going to cause regressions on old machines.

So you need to hook receive at a point outside of the IRQ layer (eg when
the workqueue starts feeding it into the ldisc), but on the tx side you
need to hook the write method of the tty really, because tty_write sends
data to the ldisc and what happens then is up to the ldisc.

Even there though you have cases that are terribly latency dependant like
some of the dumb programming tools that don't window. If you halve
someones programming rate for microcontrollers you are going to get hate
mail 8) For RX that can mostly be avoided by queuing the data then doing
the LEDs, for TX I'm not clear you can easily hide the latency.

Alan

Re: [PATCH 01/39] Annotate module params that specify hardware parameters (eg. ioport)

[One Thousand Gnomes]

> If the parameters plausibly make it possible for root to modify the 
> kernel in interesting ways, then restricting them makes sense. My gut 
> sense is that parameters that allow the alteration of the base address 
> of memory mapped devices are clearly a problem in this respect, but port 
> io and IRQs *probably* aren't. On the other hand, blocking mmio base 
> addresses and not blocking the others is kind of inconsistent.

It is actually useful even without secure boot because right now
DAC capability bypass is easier to get and gives you CAP_SYS_RAWIO if
you've mastered your first class in being an 3733t h4x0r. (because you can
modify /etc/moprobe.d/* and even with signed modules that's not
protected).

It's also the case if you go through those drivers that pretty much none
of them are found on a modern EFI and secure boot enabled system and
those that are will be using ACPI, PCI, devicetree or other reliablish
bus enumerations instead. The cross section of people needing io=foo, and
having secure boot is close to nil.

> This is a logical extension to the base patchset, and one maintainer has 
> NAKed the base patchset due to it lacking this feature. If you don't 
> care about this then just tell Alan that you want the base patchset 
> merged anyway and we'll go from there. Let's not get into a situation 
> where people are being given incompatible requirements before 
> something's merged.

The base patchset actually doesn't do anything without it. I'd hope the
vendors adopt it anyway if they are serious about secure boot (or
security in general) given it's really valuable even without the secure
boot firmware nonsense to be able to boot a machine and then lock down
raw I/O access.

Alan
 

Re: [PATCH 01/39] Annotate module params that specify hardware parameters (eg. ioport)

[One Thousand Gnomes]

> If the parameters plausibly make it possible for root to modify the 
> kernel in interesting ways, then restricting them makes sense. My gut 
> sense is that parameters that allow the alteration of the base address 
> of memory mapped devices are clearly a problem in this respect, but port 
> io and IRQs *probably* aren't. On the other hand, blocking mmio base 
> addresses and not blocking the others is kind of inconsistent.

It is actually useful even without secure boot because right now
DAC capability bypass is easier to get and gives you CAP_SYS_RAWIO if
you've mastered your first class in being an 3733t h4x0r. (because you can
modify /etc/moprobe.d/* and even with signed modules that's not
protected).

It's also the case if you go through those drivers that pretty much none
of them are found on a modern EFI and secure boot enabled system and
those that are will be using ACPI, PCI, devicetree or other reliablish
bus enumerations instead. The cross section of people needing io=foo, and
having secure boot is close to nil.

> This is a logical extension to the base patchset, and one maintainer has 
> NAKed the base patchset due to it lacking this feature. If you don't 
> care about this then just tell Alan that you want the base patchset 
> merged anyway and we'll go from there. Let's not get into a situation 
> where people are being given incompatible requirements before 
> something's merged.

The base patchset actually doesn't do anything without it. I'd hope the
vendors adopt it anyway if they are serious about secure boot (or
security in general) given it's really valuable even without the secure
boot firmware nonsense to be able to boot a machine and then lock down
raw I/O access.

Alan
 

Re: [PATCH 01/39] Annotate module params that specify hardware parameters (eg. ioport)

[One Thousand Gnomes]

On Thu, 01 Dec 2016 16:02:26 +
David Howells  wrote:

> Greg KH  wrote:
> 
> > Also, I think Alan's comment about it the last time it came up was more like
> > a "look at all of the other ways you could do bad things to hardware!"
> > comment, not a "you need to also do this thing too!" type of request.  


In all honesty I think both need to go in together, otherwise the first
patch is useless. It's not a case of "oh there may be another obscure
exploit .." , this is "I can automate it with a python script, post a
CVE, and show I'm awesome" 8)

Alan

Re: [PATCH 01/39] Annotate module params that specify hardware parameters (eg. ioport)

[One Thousand Gnomes]

On Thu, 01 Dec 2016 16:02:26 +
David Howells  wrote:

> Greg KH  wrote:
> 
> > Also, I think Alan's comment about it the last time it came up was more like
> > a "look at all of the other ways you could do bad things to hardware!"
> > comment, not a "you need to also do this thing too!" type of request.  


In all honesty I think both need to go in together, otherwise the first
patch is useless. It's not a case of "oh there may be another obscure
exploit .." , this is "I can automate it with a python script, post a
CVE, and show I'm awesome" 8)

Alan

Re: [PATCH 09/39] Annotate hardware config module parameters in drivers/i2c/

[One Thousand Gnomes]

O> > > Suggested-by: One Thousand Gnomes <gno...@lxorguk.ukuu.org.uk>  
> > 
> > I know this is only a Suggested-by and not a Signed-off-by, but still I
> > believe the Developer's Certificate of Origin applies, and it says:
> > "using your real name (sorry, no pseudonyms or anonymous
> > contributions.)"  
> 
> I asked him what he prefers - but no response.


I didn't see that question but "Alan Cox" is probably saner - the
thousand gnomes is an old Linus joke 8)
 
> > i2c-piix4.c:module_param (force, int, 0);
> > i2c-sis630.c:module_param(force, bool, 0);
> > i2c-viapro.c:module_param(force, bool, 0);  
> 
> I don't know either.  One could argue it *should* be locked down because its
> need appears to reflect a BIOS bug.

And none of those should show up in that usage form on a box new enough
to have EFI secure boot. Those that do have i2c busses will report them
via ACPI by that period.

Alan

Re: [PATCH 09/39] Annotate hardware config module parameters in drivers/i2c/

[One Thousand Gnomes]

O> > > Suggested-by: One Thousand Gnomes   
> > 
> > I know this is only a Suggested-by and not a Signed-off-by, but still I
> > believe the Developer's Certificate of Origin applies, and it says:
> > "using your real name (sorry, no pseudonyms or anonymous
> > contributions.)"  
> 
> I asked him what he prefers - but no response.


I didn't see that question but "Alan Cox" is probably saner - the
thousand gnomes is an old Linus joke 8)
 
> > i2c-piix4.c:module_param (force, int, 0);
> > i2c-sis630.c:module_param(force, bool, 0);
> > i2c-viapro.c:module_param(force, bool, 0);  
> 
> I don't know either.  One could argue it *should* be locked down because its
> need appears to reflect a BIOS bug.

And none of those should show up in that usage form on a box new enough
to have EFI secure boot. Those that do have i2c busses will report them
via ACPI by that period.

Alan

Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed

[One Thousand Gnomes]

On Tue, 29 Nov 2016 14:03:31 +
David Howells  wrote:

> How about the attached?  Obviously it need extending to other drivers.
> 
> I thought that if I'm changing the module_param annotations anyway then it's
> probably worth bunging in an extra parameter that notes what the parameter
> modifies (ioport, iomem, etc.) for future reference, even if we don't store
> it.


With a security hat on the security best practice and long standing
accepted rule is that you whitelist rather than blacklist, so there ought
to be a 

module_param_safe_array()
etc

to mark parameters that are safe, not the reverse.


That debate aside I think the patch is exactly what is needed for this,
and is probably useful for more general hardening as well.

Alan

Re: [PATCH] Lock down drivers that can have io ports, io mem, irqs and dma changed

[One Thousand Gnomes]

On Tue, 29 Nov 2016 14:03:31 +
David Howells  wrote:

> How about the attached?  Obviously it need extending to other drivers.
> 
> I thought that if I'm changing the module_param annotations anyway then it's
> probably worth bunging in an extra parameter that notes what the parameter
> modifies (ioport, iomem, etc.) for future reference, even if we don't store
> it.


With a security hat on the security best practice and long standing
accepted rule is that you whitelist rather than blacklist, so there ought
to be a 

module_param_safe_array()
etc

to mark parameters that are safe, not the reverse.


That debate aside I think the patch is exactly what is needed for this,
and is probably useful for more general hardening as well.

Alan

Re: [PATCH 00/16] Kernel lockdown

[One Thousand Gnomes]

> This applies equally to the kernel command line, and given that we
> cannot authenticate it, we should whitelist params that we know to be
> safe, and filter out all others. A similar concern exists for the
> device tree on ARM/arm64, and we already disable the DTB loader in the
> UEFI stub if secure boot is enabled.

Or you sign the boot command line.

> > Without that at least fixed I don't see the point in merging this. Either
> > we don't do it (which given the level of security the current Linux
> > kernel provides, and also all the golden key messups from elsewhere might
> > be the honest approach), or at least try and do the job right.
> >
> > Less security is better than fake security. If you've got less security
> > your take appropriate precautions. If you rely on fake security you don't.
> >  
> 
> In general, I think kernel hardening is an important topic

It is - so pushing something with known trivial holes isn't a useful way
to do this. The module parameter hole needs to be addressed before this
is fit for upstream.

Alan

Re: [PATCH 00/16] Kernel lockdown

[One Thousand Gnomes]

> This applies equally to the kernel command line, and given that we
> cannot authenticate it, we should whitelist params that we know to be
> safe, and filter out all others. A similar concern exists for the
> device tree on ARM/arm64, and we already disable the DTB loader in the
> UEFI stub if secure boot is enabled.

Or you sign the boot command line.

> > Without that at least fixed I don't see the point in merging this. Either
> > we don't do it (which given the level of security the current Linux
> > kernel provides, and also all the golden key messups from elsewhere might
> > be the honest approach), or at least try and do the job right.
> >
> > Less security is better than fake security. If you've got less security
> > your take appropriate precautions. If you rely on fake security you don't.
> >  
> 
> In general, I think kernel hardening is an important topic

It is - so pushing something with known trivial holes isn't a useful way
to do this. The module parameter hole needs to be addressed before this
is fit for upstream.

Alan

Re: [PATCH] x86/boot: Fail the boot if !M486 and CPUID is missing

[One Thousand Gnomes]

> Rather than trying to work around these issues, just have the kernel
> fail loudly if it's running on a CPUID-less 486, doesn't have CPUID,
> and doesn't have CONFIG_M486 set.

NAK

This still breaks the Geode at the very least and I think the ELAN and
some of the other older socket 7 devices. These have their own config CPU
type (and in some cases *need* it selected) but do not have CPUID.

Given the cores without CPUID are often post 486 in other respects this
seems a bit odd. In fact I can't help thinking that the problem is that
sync_core tests CONFIG_M486 whereas we should have and test

HAVE_CPUID

and set this by processor type (M586/M486/GEODEGX1/GEODE_LX1/Cyrix plus I
think ELAN not having it)

I'd been wondering why Geode wasn't working on modern kernels, now I
think I know 8)

Alan

Re: [PATCH] x86/boot: Fail the boot if !M486 and CPUID is missing

[One Thousand Gnomes]

> Rather than trying to work around these issues, just have the kernel
> fail loudly if it's running on a CPUID-less 486, doesn't have CPUID,
> and doesn't have CONFIG_M486 set.

NAK

This still breaks the Geode at the very least and I think the ELAN and
some of the other older socket 7 devices. These have their own config CPU
type (and in some cases *need* it selected) but do not have CPUID.

Given the cores without CPUID are often post 486 in other respects this
seems a bit odd. In fact I can't help thinking that the problem is that
sync_core tests CONFIG_M486 whereas we should have and test

HAVE_CPUID

and set this by processor type (M586/M486/GEODEGX1/GEODE_LX1/Cyrix plus I
think ELAN not having it)

I'd been wondering why Geode wasn't working on modern kernels, now I
think I know 8)

Alan

Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available

[One Thousand Gnomes]

> > increase in timestamp resoultion of at least another 10e-3 is
> > likely  
> 
> Is it, though?  To be useful, surely you have to be able to jam quite a few
> instructions into a 1ns block, including memory accesses.
> 
> Rather than providing:
> 
>   struct timestamp {
>   __s64 seconds;
>   __s64 femtoseconds;
>   };
> 
> which would require 64-bit divisions to get nanosecond timestamps that we do
> actually use, I would lean towards:
> 
>   struct timestamp {
>   __s64 seconds;
>   __s32 nanoseconds;
>   __s32 femtoseconds;
>   };

Which gets silly. The nanosecond world is defined by the speed of light.
Short of someone finding a way to change that digital computing as we
know it today is going to be living in the nanoseconds world. You hit the
point of 'can't measure the difference' before you hit the point of 'can
usefully order things using'

Alan

Re: [PATCH 1/4] statx: Add a system call to make enhanced file info available

[One Thousand Gnomes]

> > increase in timestamp resoultion of at least another 10e-3 is
> > likely  
> 
> Is it, though?  To be useful, surely you have to be able to jam quite a few
> instructions into a 1ns block, including memory accesses.
> 
> Rather than providing:
> 
>   struct timestamp {
>   __s64 seconds;
>   __s64 femtoseconds;
>   };
> 
> which would require 64-bit divisions to get nanosecond timestamps that we do
> actually use, I would lean towards:
> 
>   struct timestamp {
>   __s64 seconds;
>   __s32 nanoseconds;
>   __s32 femtoseconds;
>   };

Which gets silly. The nanosecond world is defined by the speed of light.
Short of someone finding a way to change that digital computing as we
know it today is going to be living in the nanoseconds world. You hit the
point of 'can't measure the difference' before you hit the point of 'can
usefully order things using'

Alan

Re: [PATCHv0 1/1] fbdev: add Intel FPGA FRAME BUFFER driver

[One Thousand Gnomes]

> AIUI, we're not taking new FB drivers. This should be a DRM driver 
> instead.

Yes - clone one of the dumb DRM drivers, or if you've got any little bits
of acceleration (even rolling the display) then it's possibly worth
accelerating for text mode.

> > +- max-width: The width of the framebuffer in pixels.
> > +- max-height: The height of the framebuffer in pixels.
> > +- bits-per-color: only "8" is currently supported  
> 
> These are not h/w properties.

How are the max ones not hardware properties ?

Alan

Re: [PATCHv0 1/1] fbdev: add Intel FPGA FRAME BUFFER driver

[One Thousand Gnomes]

> AIUI, we're not taking new FB drivers. This should be a DRM driver 
> instead.

Yes - clone one of the dumb DRM drivers, or if you've got any little bits
of acceleration (even rolling the display) then it's possibly worth
accelerating for text mode.

> > +- max-width: The width of the framebuffer in pixels.
> > +- max-height: The height of the framebuffer in pixels.
> > +- bits-per-color: only "8" is currently supported  
> 
> These are not h/w properties.

How are the max ones not hardware properties ?

Alan

Re: why is the sys_close symbol exported ?

[One Thousand Gnomes]

On Fri, 18 Nov 2016 09:56:52 +0100
jmfriedt  wrote:

> Following the various rootkit and system call redirection developments, the 
> current
> way of identifying the location of the system call table seems to be brute 
> force scanning 
> the memory for the location of one of the system calls. This is only possible 
> from a module
> if the symbol is exported: I see that only one system call symbol is still 
> exported, that
> is sys_close. Removing this symbol export would hinder one of the ways of 
> finding the 
> systam call table: I have not been able to find the reason for exporting this 
> particular
> symbol (while sys_open for example is not exported). Can anyone justify why 
> that is ?
> 
> Thank you, Jean-Michel
> 

find . -name "*.[ch]" -exec grep -H sys_close {} \;

So currently it is needed by autofs4, binfmt_misc, net/kcm/kcmsock and
those look like legitimate use cases.

It might be worth changing sys_close to just wrap a call to
do_sys_close() which is the existing code. That would make it slightly
harder.

That said anyone doing syscall table scanning that would can do it at
least two other pretty reliable ways by working form the system call
entry point, which is trivially discoverable.

It's one reason I'd really like to see kvm/qemu provide 'read only until
the virtual machine exits' memory range so that you can irrevocably
protect page ranges (like the syscalls and much of the kernel code)
within a VM.

Alan

Re: why is the sys_close symbol exported ?

[One Thousand Gnomes]

On Fri, 18 Nov 2016 09:56:52 +0100
jmfriedt  wrote:

> Following the various rootkit and system call redirection developments, the 
> current
> way of identifying the location of the system call table seems to be brute 
> force scanning 
> the memory for the location of one of the system calls. This is only possible 
> from a module
> if the symbol is exported: I see that only one system call symbol is still 
> exported, that
> is sys_close. Removing this symbol export would hinder one of the ways of 
> finding the 
> systam call table: I have not been able to find the reason for exporting this 
> particular
> symbol (while sys_open for example is not exported). Can anyone justify why 
> that is ?
> 
> Thank you, Jean-Michel
> 

find . -name "*.[ch]" -exec grep -H sys_close {} \;

So currently it is needed by autofs4, binfmt_misc, net/kcm/kcmsock and
those look like legitimate use cases.

It might be worth changing sys_close to just wrap a call to
do_sys_close() which is the existing code. That would make it slightly
harder.

That said anyone doing syscall table scanning that would can do it at
least two other pretty reliable ways by working form the system call
entry point, which is trivially discoverable.

It's one reason I'd really like to see kvm/qemu provide 'read only until
the virtual machine exits' memory range so that you can irrevocably
protect page ranges (like the syscalls and much of the kernel code)
within a VM.

Alan

Re: [RFC][PATCH 0/4] Enhanced file stat system call

[One Thousand Gnomes]

> Hmmm... Interesting question.  Probably should.  But you could be insane and
> RAID an nbd and a local disk.  Further, does NFS over a loopback device to
> nfsd on the same machine qualify as root?  What if that's exposing a local fs
> on NBD?  Perhaps I should drop 'REMOTE' for now.  It sounds like something
> that a GUI filemanager might find interesting, though.

GUI file managers already try and guess some of this in order to decide
whether to display icons off remote file systems.

You could be insane but it's always going to be a hint and nothing more
and if it can't be perfect then that just goes in the notes in the manual
page.

Alan

Re: [RFC][PATCH 0/4] Enhanced file stat system call

[One Thousand Gnomes]

> Hmmm... Interesting question.  Probably should.  But you could be insane and
> RAID an nbd and a local disk.  Further, does NFS over a loopback device to
> nfsd on the same machine qualify as root?  What if that's exposing a local fs
> on NBD?  Perhaps I should drop 'REMOTE' for now.  It sounds like something
> that a GUI filemanager might find interesting, though.

GUI file managers already try and guess some of this in order to decide
whether to display icons off remote file systems.

You could be insane but it's always going to be a hint and nothing more
and if it can't be perfect then that just goes in the notes in the manual
page.

Alan

Re: [RFC][PATCH 0/4] Enhanced file stat system call

[One Thousand Gnomes]

>  (2) Lightweight stat (AT_STATX_DONT_SYNC): Ask for just those details of
>  interest, and allow a network fs to approximate anything not of
>  interest, without going to the server.
> 
>  (3) Heavyweight stat (AT_STATX_FORCE_SYNC): Force a network fs to flush
>  buffers and go to the server, even if it thinks its cached attributes
>  are up to date.

That seems an odd way to do it. Wouldn't it be cleaner and more flexible
to give a timestamp of the oldest time you consider acceptable (and
obviously passing 0 indicates whatever you have)

>  (4) Allow the filesystem to indicate what it can/cannot provide: A
>  filesystem can now say it doesn't support a standard stat feature if
>  that isn't available.
> 
>  (5) Make the fields a consistent size on all arches, and make them large.
> 
>  (6) Can be extended by using more request flags and using up the padding
>  space in the statx struct.
> 
> Note that no lstat() equivalent is required as that can be implemented
> through statx() with atflag == 0.  There is also no fstat() equivalent as
> that can be implemented through statx() with filename == NULL and the
> relevant fd passed as dfd.

and dfd + a name gives you fstatat() ? The cover note could be clearer on
this.

Should the fields really be split the way they are for times rather than
a struct for each one so you can write code generically to handle one of
those rather than having to have a 4 way switch statement all the time.

Another attribute that would be nice (but migt need some trivial device
layer tweaking) would be STATX_ATTR_VOLATILE for filesystems that will
probably evaporate on a reboot. That's useful information for tools like
installers and also for sanity checking things like backup paths.

Remote needs to have clear semantics: is ext4fs over nbd 'remote' for
example ?

Alan

Re: [RFC][PATCH 0/4] Enhanced file stat system call

[One Thousand Gnomes]

>  (2) Lightweight stat (AT_STATX_DONT_SYNC): Ask for just those details of
>  interest, and allow a network fs to approximate anything not of
>  interest, without going to the server.
> 
>  (3) Heavyweight stat (AT_STATX_FORCE_SYNC): Force a network fs to flush
>  buffers and go to the server, even if it thinks its cached attributes
>  are up to date.

That seems an odd way to do it. Wouldn't it be cleaner and more flexible
to give a timestamp of the oldest time you consider acceptable (and
obviously passing 0 indicates whatever you have)

>  (4) Allow the filesystem to indicate what it can/cannot provide: A
>  filesystem can now say it doesn't support a standard stat feature if
>  that isn't available.
> 
>  (5) Make the fields a consistent size on all arches, and make them large.
> 
>  (6) Can be extended by using more request flags and using up the padding
>  space in the statx struct.
> 
> Note that no lstat() equivalent is required as that can be implemented
> through statx() with atflag == 0.  There is also no fstat() equivalent as
> that can be implemented through statx() with filename == NULL and the
> relevant fd passed as dfd.

and dfd + a name gives you fstatat() ? The cover note could be clearer on
this.

Should the fields really be split the way they are for times rather than
a struct for each one so you can write code generically to handle one of
those rather than having to have a 4 way switch statement all the time.

Another attribute that would be nice (but migt need some trivial device
layer tweaking) would be STATX_ATTR_VOLATILE for filesystems that will
probably evaporate on a reboot. That's useful information for tools like
installers and also for sanity checking things like backup paths.

Remote needs to have clear semantics: is ext4fs over nbd 'remote' for
example ?

Alan

Re: [PATCH 00/16] Kernel lockdown

[One Thousand Gnomes]

Whether it's a good idea aside

You need to filter or lock down kernel module options because a lot of
modules let you set the I/O port or similar (eg mmio) which means you can
hack the entire machine with say the 8250 driver just by using it with an
mmio of the right location to patch the secure state to zero just by
getting the ability to write to the modules conf file.

Without that at least fixed I don't see the point in merging this. Either
we don't do it (which given the level of security the current Linux
kernel provides, and also all the golden key messups from elsewhere might
be the honest approach), or at least try and do the job right.

Less security is better than fake security. If you've got less security
your take appropriate precautions. If you rely on fake security you don't.

The two other nasty cases you miss should be fine for x86 secure boot -
but maybe not for secure boot in general. That is firmware loading and
initial firewire state. Both should be fine on any 'secure' boot PC.


Alan

Re: [PATCH 00/16] Kernel lockdown

[One Thousand Gnomes]

Whether it's a good idea aside

You need to filter or lock down kernel module options because a lot of
modules let you set the I/O port or similar (eg mmio) which means you can
hack the entire machine with say the 8250 driver just by using it with an
mmio of the right location to patch the secure state to zero just by
getting the ability to write to the modules conf file.

Without that at least fixed I don't see the point in merging this. Either
we don't do it (which given the level of security the current Linux
kernel provides, and also all the golden key messups from elsewhere might
be the honest approach), or at least try and do the job right.

Less security is better than fake security. If you've got less security
your take appropriate precautions. If you rely on fake security you don't.

The two other nasty cases you miss should be fine for x86 secure boot -
but maybe not for secure boot in general. That is firmware loading and
initial firewire state. Both should be fine on any 'secure' boot PC.


Alan

Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA

[One Thousand Gnomes]

> > It's not a safe assumption for x86 at least. There are a few systems with
> > multiple ISA busses particularly older laptops with a docking station.  
> 
> But do they have multiple ISA domains? There is no real harm in supporting
> it, the (small) downsides I can think of are:

I don't believe they x86 class ones have multiple ISA domains. But as
I've said I don't know how the electronics in the older ThinkPad worked
when it used two PIIX4s with some LPC or ISA stuff on each.

It works in DOS and unmodified Linux so I'm pretty sure there are no
additional domains. Likewise the various x86 schemes that route some bits
of ISA bus off into strange places work in DOS and don't have any
overlaps.

yenta_socket handles PCI/PCMCIA bridging and routes a range of that flat
ISA space appropriately to the card.

Alan

Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA

[One Thousand Gnomes]

> > It's not a safe assumption for x86 at least. There are a few systems with
> > multiple ISA busses particularly older laptops with a docking station.  
> 
> But do they have multiple ISA domains? There is no real harm in supporting
> it, the (small) downsides I can think of are:

I don't believe they x86 class ones have multiple ISA domains. But as
I've said I don't know how the electronics in the older ThinkPad worked
when it used two PIIX4s with some LPC or ISA stuff on each.

It works in DOS and unmodified Linux so I'm pretty sure there are no
additional domains. Likewise the various x86 schemes that route some bits
of ISA bus off into strange places work in DOS and don't have any
overlaps.

yenta_socket handles PCI/PCMCIA bridging and routes a range of that flat
ISA space appropriately to the card.

Alan

Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

[One Thousand Gnomes]

On Wed, 09 Nov 2016 22:34:38 +0100
Arnd Bergmann  wrote:

> On Wednesday, November 9, 2016 12:10:43 PM CET Gabriele Paoloni wrote:
> > > On Tuesday, November 8, 2016 11:47:09 AM CET zhichang.yuan wrote:  
> > > > +   /*
> > > > +* The first PCIBIOS_MIN_IO is reserved specifically for  
> > > indirectIO.  
> > > > +* It will separate indirectIO range from pci host bridge to
> > > > +* avoid the possible PIO conflict.
> > > > +* Set the indirectIO range directly here.
> > > > +*/
> > > > +   lpcdev->io_ops.start = 0;
> > > > +   lpcdev->io_ops.end = PCIBIOS_MIN_IO - 1;
> > > > +   lpcdev->io_ops.devpara = lpcdev;
> > > > +   lpcdev->io_ops.pfin = hisilpc_comm_in;
> > > > +   lpcdev->io_ops.pfout = hisilpc_comm_out;
> > > > +   lpcdev->io_ops.pfins = hisilpc_comm_ins;
> > > > +   lpcdev->io_ops.pfouts = hisilpc_comm_outs;  
> > > 
> > > I have to look at patch 2 in more detail again, after missing a few
> > > review
> > > rounds. I'm still a bit skeptical about hardcoding a logical I/O port
> > > range here, and would hope that we can just go through the same
> > > assignment of logical port ranges that we have for PCI buses,
> > > decoupling
> > > the bus addresses from the linux-internal ones.  
> > 
> > The point here is that we want to avoid any conflict/overlap between
> > the LPC I/O space and the PCI I/O space. With the assignment above
> > we make sure that LPC never interfere with PCI I/O space.  
> 
> But we already abstract the PCI I/O space using dynamic registration.
> There is no need to hardcode the logical address for ISA, though
> I think we can hardcode the bus address to start at zero here.

Pedantically ISA starts at 0x100. The LPC may start at 0x00 as it also
covers motherboard devices (0x00-0xFF). It is also possible that the
'LPC' space is only partially routed to the PCI bridges because some if
it magially disappears on CPU die (at least on x86) and has done since
the era of socket 7 (eg the Cyrix 6x86 doesn't route 0x22/0x23 out of the
CPU).

Assuming LPC starts at 0 ought to be ok given the PCI root bridge
shouldn't see the transactions.

The LPC or it's equivalent may also not be routed via the PCI bridges at
all, so you could have an LPC mapping that is unused or partially used
with another bus actually getting some classes of LPC traffic - on x86 at
least.

Alan

Re: [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06

[One Thousand Gnomes]

On Wed, 09 Nov 2016 22:34:38 +0100
Arnd Bergmann  wrote:

> On Wednesday, November 9, 2016 12:10:43 PM CET Gabriele Paoloni wrote:
> > > On Tuesday, November 8, 2016 11:47:09 AM CET zhichang.yuan wrote:  
> > > > +   /*
> > > > +* The first PCIBIOS_MIN_IO is reserved specifically for  
> > > indirectIO.  
> > > > +* It will separate indirectIO range from pci host bridge to
> > > > +* avoid the possible PIO conflict.
> > > > +* Set the indirectIO range directly here.
> > > > +*/
> > > > +   lpcdev->io_ops.start = 0;
> > > > +   lpcdev->io_ops.end = PCIBIOS_MIN_IO - 1;
> > > > +   lpcdev->io_ops.devpara = lpcdev;
> > > > +   lpcdev->io_ops.pfin = hisilpc_comm_in;
> > > > +   lpcdev->io_ops.pfout = hisilpc_comm_out;
> > > > +   lpcdev->io_ops.pfins = hisilpc_comm_ins;
> > > > +   lpcdev->io_ops.pfouts = hisilpc_comm_outs;  
> > > 
> > > I have to look at patch 2 in more detail again, after missing a few
> > > review
> > > rounds. I'm still a bit skeptical about hardcoding a logical I/O port
> > > range here, and would hope that we can just go through the same
> > > assignment of logical port ranges that we have for PCI buses,
> > > decoupling
> > > the bus addresses from the linux-internal ones.  
> > 
> > The point here is that we want to avoid any conflict/overlap between
> > the LPC I/O space and the PCI I/O space. With the assignment above
> > we make sure that LPC never interfere with PCI I/O space.  
> 
> But we already abstract the PCI I/O space using dynamic registration.
> There is no need to hardcode the logical address for ISA, though
> I think we can hardcode the bus address to start at zero here.

Pedantically ISA starts at 0x100. The LPC may start at 0x00 as it also
covers motherboard devices (0x00-0xFF). It is also possible that the
'LPC' space is only partially routed to the PCI bridges because some if
it magially disappears on CPU die (at least on x86) and has done since
the era of socket 7 (eg the Cyrix 6x86 doesn't route 0x22/0x23 out of the
CPU).

Assuming LPC starts at 0 ought to be ok given the PCI root bridge
shouldn't see the transactions.

The LPC or it's equivalent may also not be routed via the PCI bridges at
all, so you could have an LPC mapping that is unused or partially used
with another bus actually getting some classes of LPC traffic - on x86 at
least.

Alan

Re: [PATCH 0/4] x86: enable User-Mode Instruction Prevention

[One Thousand Gnomes]

> I took a closer look at the dosemu code. It appears that it does not

That doesn't tell you want DOS itself will try and do...

> purposely utilize SGDT to obtain the descriptor table while in vm86. It
> does use SGDT (in protected mode) to emulate certain functionality such
> as the Virtual xxx Driver. In such a case, UMIP needs to be disabled.
> However, this code seems to be disabled [1]. dosemu includes an i386
> emulator that in some cases uses the actual instructions of the host
> system. In such cases, UMIP might be needed to be disabled.

Is anyone actually still using DOSemu these days or are people all
using DOSbox ?

Alan

Re: [PATCH 0/4] x86: enable User-Mode Instruction Prevention

[One Thousand Gnomes]

> I took a closer look at the dosemu code. It appears that it does not

That doesn't tell you want DOS itself will try and do...

> purposely utilize SGDT to obtain the descriptor table while in vm86. It
> does use SGDT (in protected mode) to emulate certain functionality such
> as the Virtual xxx Driver. In such a case, UMIP needs to be disabled.
> However, this code seems to be disabled [1]. dosemu includes an i386
> emulator that in some cases uses the actual instructions of the host
> system. In such cases, UMIP might be needed to be disabled.

Is anyone actually still using DOSemu these days or are people all
using DOSbox ?

Alan

Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA

[One Thousand Gnomes]

> I think it is a relatively safe assumption that there is only one
> ISA bridge. A lot of old drivers hardcode PIO or memory addresses

It's not a safe assumption for x86 at least. There are a few systems with
multiple ISA busses particularly older laptops with a docking station.

> when talking to an ISA device, so having multiple instances is
> already problematic.

PCMCIA devices handle it themselves so are ok. I'm not clear how the dual
PIIX4 configuration used in the older IBM laptop docks actually worked so
I assume the transaction went out of both bridges and providing one of
them responded the other kept silent as you simply stuffed the card into
the dock and it worked.

Alan

Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA

[One Thousand Gnomes]

> I think it is a relatively safe assumption that there is only one
> ISA bridge. A lot of old drivers hardcode PIO or memory addresses

It's not a safe assumption for x86 at least. There are a few systems with
multiple ISA busses particularly older laptops with a docking station.

> when talking to an ISA device, so having multiple instances is
> already problematic.

PCMCIA devices handle it themselves so are ok. I'm not clear how the dual
PIIX4 configuration used in the older IBM laptop docks actually worked so
I assume the transaction went out of both bridges and providing one of
them responded the other kept silent as you simply stuffed the card into
the dock and it worked.

Alan

Re: Bogus "APIC: NR_CPUS/possible_cpus limit of 4 reached" messages

[One Thousand Gnomes]

On Thu, 6 Oct 2016 13:27:37 +0200
Markus Trippelsdorf  wrote:

> On current trunk I get during boot:
> 
> [0.00] APIC: NR_CPUS/possible_cpus limit of 4 reached.  Processor 
> 4/0x84 ignored.
> [0.00] APIC: NR_CPUS/possible_cpus limit of 4 reached.  Processor 
> 5/0x85 ignored.
> 
> I don't think these messages make much sense on a 4-core machine.
> 

Four cores with or without hyperthreading ?

Alan

Re: Bogus "APIC: NR_CPUS/possible_cpus limit of 4 reached" messages

[One Thousand Gnomes]

On Thu, 6 Oct 2016 13:27:37 +0200
Markus Trippelsdorf  wrote:

> On current trunk I get during boot:
> 
> [0.00] APIC: NR_CPUS/possible_cpus limit of 4 reached.  Processor 
> 4/0x84 ignored.
> [0.00] APIC: NR_CPUS/possible_cpus limit of 4 reached.  Processor 
> 5/0x85 ignored.
> 
> I don't think these messages make much sense on a 4-core machine.
> 

Four cores with or without hyperthreading ?

Alan

Re: Should drivers like nvme let userspace control their latency via dev_pm_qos?

[One Thousand Gnomes]

On Fri, 16 Sep 2016 08:26:03 -0700
Andy Lutomirski  wrote:

> I'm adding power management to the nvme driver, and I'm exposing
> exactly one knob via sysfs: the maximum permissible latency.  This
> isn't a power domain issue, and it has no dependencies -- it's
> literally just the maximum latency that the driver may impose on I/O
> for power saving purposes.

Why is this in the driver. Surely the latency is a property of the
request queue and the requests being made. Now it may well be that its
implement as min(list-of-queues) but a device sysfs node seems a strange
place to stick it.

Alan

Re: Should drivers like nvme let userspace control their latency via dev_pm_qos?

[One Thousand Gnomes]

On Fri, 16 Sep 2016 08:26:03 -0700
Andy Lutomirski  wrote:

> I'm adding power management to the nvme driver, and I'm exposing
> exactly one knob via sysfs: the maximum permissible latency.  This
> isn't a power domain issue, and it has no dependencies -- it's
> literally just the maximum latency that the driver may impose on I/O
> for power saving purposes.

Why is this in the driver. Surely the latency is a property of the
request queue and the requests being made. Now it may well be that its
implement as min(list-of-queues) but a device sysfs node seems a strange
place to stick it.

Alan

Re: [RFC] writev() semantics with invalid iovec in the middle

[One Thousand Gnomes]

> Unfortunately, some of LTP writev tests end up checking that writev() does
> behave that way - they feed it a three-element iovec with shorter-than-page
> segments, the second of which is all invalid.  And they check that the
> entire first segment had been written.

1003.1 says

"Each iovec entry specifies the base address and length of an area in
memory from which data should be written. The writev() function shall
always write a complete area before proceeding to the next."

and I imagine that is what LTP is attempting to test.

The moment you pass an invalid address you are in the land of undefined
behaviour, so I would read the standard as actually trying to deal with
the behaviour in defined situations (eg out of disk space mid writev()).

Alan

Re: [RFC] writev() semantics with invalid iovec in the middle

[One Thousand Gnomes]

> Unfortunately, some of LTP writev tests end up checking that writev() does
> behave that way - they feed it a three-element iovec with shorter-than-page
> segments, the second of which is all invalid.  And they check that the
> entire first segment had been written.

1003.1 says

"Each iovec entry specifies the base address and length of an area in
memory from which data should be written. The writev() function shall
always write a complete area before proceeding to the next."

and I imagine that is what LTP is attempting to test.

The moment you pass an invalid address you are in the land of undefined
behaviour, so I would read the standard as actually trying to deal with
the behaviour in defined situations (eg out of disk space mid writev()).

Alan

Re: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)

[One Thousand Gnomes]

On Thu, 15 Sep 2016 16:19:52 +0300
Andrey Utkin  wrote:

> On Thu, Sep 15, 2016 at 03:15:53PM +0200, Hans Verkuil wrote:
> > It could be related to the fact that a PCI write may be delayed unless
> > it is followed by a read (see also the comments in 
> > drivers/media/pci/ivtv/ivtv-driver.h).  
> 
> Thanks for explanation!
> 
> > That was probably the reason for the pci_read_config_word in the reg_write
> > code. Try putting that back (and just that).  
> 
> In this case reg_write becomes not atomic, thus spinlock would be
> required again here, right?

No - PCI writes are ordered but may not complete until the next read or
config access. That ordering isn't affected by things like spin locking
as it is a property of the bus.

Usually this only matters in obscure cases - timing is one of them, and
the other is when freeing memory because writes are posted both ways so
you need to write to stop the transfer, read to ensure the transfer has
completed and then free the target memory.

Alan

Re: solo6010 modprobe lockup since e1ceb25a (v4.3 regression)

[One Thousand Gnomes]

On Thu, 15 Sep 2016 16:19:52 +0300
Andrey Utkin  wrote:

> On Thu, Sep 15, 2016 at 03:15:53PM +0200, Hans Verkuil wrote:
> > It could be related to the fact that a PCI write may be delayed unless
> > it is followed by a read (see also the comments in 
> > drivers/media/pci/ivtv/ivtv-driver.h).  
> 
> Thanks for explanation!
> 
> > That was probably the reason for the pci_read_config_word in the reg_write
> > code. Try putting that back (and just that).  
> 
> In this case reg_write becomes not atomic, thus spinlock would be
> required again here, right?

No - PCI writes are ordered but may not complete until the next read or
config access. That ordering isn't affected by things like spin locking
as it is a property of the bus.

Usually this only matters in obscure cases - timing is one of them, and
the other is when freeing memory because writes are posted both ways so
you need to write to stop the transfer, read to ensure the transfer has
completed and then free the target memory.

Alan

Re: [PATCH v2 0/8] Support Intel® Turbo Boost Max Technology 3.0

[One Thousand Gnomes]

On Thu, 15 Sep 2016 14:14:30 +0200
Pavel Machek  wrote:

> Hi!
> 
> > - Feature is enabled by default for single socket systems
> > 
> > With Intel® Turbo Boost Max Technology 3.0 (ITMT), single-threaded 
> > performance is
> > optimized by identifying processor's fastest core and running critical 
> > workloads
> > on it.
> > Refere to:
> > http://www.intel.com/content/www/us/en/architecture-and-technology/turbo-boost/turbo-boost-max-technology.html
> >   
> 
> That does not really explain much.
> 
> How does it work? Do the different cores have different max
> frequencies due to manufacturing differences? Ot is it running the
> workload on coldest core?

That's all down to the CPU, not architectural and may change.

The ACPI tables describe which cores to use, whether that relates to
manufacturing, positioning or whatever isn't exposed.

Alan

Re: [PATCH v2 0/8] Support Intel® Turbo Boost Max Technology 3.0

[One Thousand Gnomes]

On Thu, 15 Sep 2016 14:14:30 +0200
Pavel Machek  wrote:

> Hi!
> 
> > - Feature is enabled by default for single socket systems
> > 
> > With Intel® Turbo Boost Max Technology 3.0 (ITMT), single-threaded 
> > performance is
> > optimized by identifying processor's fastest core and running critical 
> > workloads
> > on it.
> > Refere to:
> > http://www.intel.com/content/www/us/en/architecture-and-technology/turbo-boost/turbo-boost-max-technology.html
> >   
> 
> That does not really explain much.
> 
> How does it work? Do the different cores have different max
> frequencies due to manufacturing differences? Ot is it running the
> workload on coldest core?

That's all down to the CPU, not architectural and may change.

The ACPI tables describe which cores to use, whether that relates to
manufacturing, positioning or whatever isn't exposed.

Alan

Re: [PATCH] pty: make ptmx file ops read-only after init

[One Thousand Gnomes]

On Wed, 14 Sep 2016 09:59:42 +0200
Jiri Slaby  wrote:

> On 09/09/2016, 12:35 AM, Kees Cook wrote:
> > The ptmx_fops structure is only changed during init, so mark it as such.  
> 
> Right, but I am missing what is the benefit? You would have to elaborate
> here...

The pages end up marked read only even to the kernel (and in future could
even be marked read only forever when in kvm if we get suitable virtual
machine extensions). That makes it much harder to patch those vectors
when making security attacks.

Re: [PATCH] pty: make ptmx file ops read-only after init

[One Thousand Gnomes]

On Wed, 14 Sep 2016 09:59:42 +0200
Jiri Slaby  wrote:

> On 09/09/2016, 12:35 AM, Kees Cook wrote:
> > The ptmx_fops structure is only changed during init, so mark it as such.  
> 
> Right, but I am missing what is the benefit? You would have to elaborate
> here...

The pages end up marked read only even to the kernel (and in future could
even be marked read only forever when in kvm if we get suitable virtual
machine extensions). That makes it much harder to patch those vectors
when making security attacks.

Re: [PATCH 0/9] tty: tty_struct dependency clean-ups

[One Thousand Gnomes]

On Sun, 11 Sep 2016 22:05:07 -0500
Rob Herring <r...@kernel.org> wrote:

> On Sun, Sep 11, 2016 at 4:14 PM, One Thousand Gnomes
> <gno...@lxorguk.ukuu.org.uk> wrote:
> > On Fri,  9 Sep 2016 17:37:01 -0500
> > Rob Herring <r...@kernel.org> wrote:
> >  
> >> This patch series removes or prepares to remove some of the dependencies
> >> on tty_struct within tty_port drivers. This will allow using tty_ports
> >> directly for so called UART slave devices.  
> >
> > You can create a tty_struct kernel side with the two tiny changes I
> > posted before. Why do you want to do invasive tree wide changes when you
> > can do simple ones ?  
> 
> Well, I don't want to do invasive changes, but I thought the idea was
> to use tty_port struct without a tty_struct.

I posted some tiny patches to break the file/tty requirement in the base
tty code for comment a while ago and they were very tiny for most ldiscs
(n_tty unsurprisingly wouldn't work this way but does anyone need kernel
mode n_tty ?)

Moving termios into the tty_port is IMHO a good thing to do whichever
approach is taken.

> I was planning to keep termios out of tty_port and make clients of
> tty_port carry it if for nothing else not quite understanding all the
> details around the lifetime, init and locking of it. If there's always
> a tty_struct then there's not much point moving it other than which
> struct makes more sense. But that would cause some churn.

The termios lifetime is the lifetime of the port, although it may get
reset at some times.

Alan

Re: [PATCH 0/9] tty: tty_struct dependency clean-ups

[One Thousand Gnomes]

On Sun, 11 Sep 2016 22:05:07 -0500
Rob Herring  wrote:

> On Sun, Sep 11, 2016 at 4:14 PM, One Thousand Gnomes
>  wrote:
> > On Fri,  9 Sep 2016 17:37:01 -0500
> > Rob Herring  wrote:
> >  
> >> This patch series removes or prepares to remove some of the dependencies
> >> on tty_struct within tty_port drivers. This will allow using tty_ports
> >> directly for so called UART slave devices.  
> >
> > You can create a tty_struct kernel side with the two tiny changes I
> > posted before. Why do you want to do invasive tree wide changes when you
> > can do simple ones ?  
> 
> Well, I don't want to do invasive changes, but I thought the idea was
> to use tty_port struct without a tty_struct.

I posted some tiny patches to break the file/tty requirement in the base
tty code for comment a while ago and they were very tiny for most ldiscs
(n_tty unsurprisingly wouldn't work this way but does anyone need kernel
mode n_tty ?)

Moving termios into the tty_port is IMHO a good thing to do whichever
approach is taken.

> I was planning to keep termios out of tty_port and make clients of
> tty_port carry it if for nothing else not quite understanding all the
> details around the lifetime, init and locking of it. If there's always
> a tty_struct then there's not much point moving it other than which
> struct makes more sense. But that would cause some churn.

The termios lifetime is the lifetime of the port, although it may get
reset at some times.

Alan

Re: [PATCH 9/9] tty: serial_core: add tty NULL check in uart_port_startup

[One Thousand Gnomes]

On Fri,  9 Sep 2016 17:37:10 -0500
Rob Herring  wrote:

> If we don't have a tty when calling uart_port_startup, skip the baudrate
> and modem control setup which depend on tty->termios struct. Either
> tty_port clients will configure the line in a separate call or we'll
> move termios into the tty_port.

I don't think getting rid of the tty object makes any sense whatsoever
just create a kernel one.

Either way the giant churny patch and this hack should be deferred in
favour of moving the termios structure into the tty port (and the locks
for it) and doing the right job once.

The termios does I agree belong in the tty_port so irrespective of final
direction fixing that is a right move.

Alan

Re: [PATCH 9/9] tty: serial_core: add tty NULL check in uart_port_startup

[One Thousand Gnomes]

On Fri,  9 Sep 2016 17:37:10 -0500
Rob Herring  wrote:

> If we don't have a tty when calling uart_port_startup, skip the baudrate
> and modem control setup which depend on tty->termios struct. Either
> tty_port clients will configure the line in a separate call or we'll
> move termios into the tty_port.

I don't think getting rid of the tty object makes any sense whatsoever
just create a kernel one.

Either way the giant churny patch and this hack should be deferred in
favour of moving the termios structure into the tty port (and the locks
for it) and doing the right job once.

The termios does I agree belong in the tty_port so irrespective of final
direction fixing that is a right move.

Alan

Re: [PATCH 4/9] tty: move TTY_IO_ERROR flag to tty_port iflags

[One Thousand Gnomes]

On Fri,  9 Sep 2016 17:37:05 -0500
Rob Herring  wrote:

> TTY_IO_ERROR is a property of the tty port rather than the tty, so move
> it to tty_port struct and remove another dependency on tty_struct from
> drivers.
> 
> Partially converted with coccinelle:
> 
> @@
> identifier t;
> identifier func;
> @@
> - func(TTY_IO_ERROR, >flags)
> + func(TTY_PORT_IO_ERROR, >port->iflags)
> 
> @@
> expression port;
> identifier func;
> @@
> - func(TTY_IO_ERROR, >flags)
> + func(TTY_PORT_IO_ERROR, )

Again this makes sense to move the object to the right structure.

I'm not convinced your recipe is a correct and reliable translation
because you don't capture the flag being cleared if the tty object is
freed and then a new one allocated. That needs further review IMHO

Alan

Re: [PATCH 4/9] tty: move TTY_IO_ERROR flag to tty_port iflags

[One Thousand Gnomes]

On Fri,  9 Sep 2016 17:37:05 -0500
Rob Herring  wrote:

> TTY_IO_ERROR is a property of the tty port rather than the tty, so move
> it to tty_port struct and remove another dependency on tty_struct from
> drivers.
> 
> Partially converted with coccinelle:
> 
> @@
> identifier t;
> identifier func;
> @@
> - func(TTY_IO_ERROR, >flags)
> + func(TTY_PORT_IO_ERROR, >port->iflags)
> 
> @@
> expression port;
> identifier func;
> @@
> - func(TTY_IO_ERROR, >flags)
> + func(TTY_PORT_IO_ERROR, )

Again this makes sense to move the object to the right structure.

I'm not convinced your recipe is a correct and reliable translation
because you don't capture the flag being cleared if the tty object is
freed and then a new one allocated. That needs further review IMHO

Alan

Re: [PATCH 0/9] tty: tty_struct dependency clean-ups

[One Thousand Gnomes]

On Fri,  9 Sep 2016 17:37:01 -0500
Rob Herring  wrote:

> This patch series removes or prepares to remove some of the dependencies 
> on tty_struct within tty_port drivers. This will allow using tty_ports 
> directly for so called UART slave devices. 

You can create a tty_struct kernel side with the two tiny changes I
posted before. Why do you want to do invasive tree wide changes when you
can do simple ones ?

> Next up after this are moving some functions to the tty_port ops. I've 
> got some WIP patches for some of that, but nothing ready to send out 
> quite yet.

I think before this lot happens you need to decide where these structures
belong. Termios and termios_locked for example could live in the tty_port
as the physical tty is incapable of having multiple sets of terminal data
at once.

Really though this looks to me like *MASSIVE* churn for now purpose.
Create a tty_struct kernel side, and use that, the needed patch is then
tiny.

so IMHO NAK

Alan

Re: [PATCH 0/9] tty: tty_struct dependency clean-ups

[One Thousand Gnomes]

On Fri,  9 Sep 2016 17:37:01 -0500
Rob Herring  wrote:

> This patch series removes or prepares to remove some of the dependencies 
> on tty_struct within tty_port drivers. This will allow using tty_ports 
> directly for so called UART slave devices. 

You can create a tty_struct kernel side with the two tiny changes I
posted before. Why do you want to do invasive tree wide changes when you
can do simple ones ?

> Next up after this are moving some functions to the tty_port ops. I've 
> got some WIP patches for some of that, but nothing ready to send out 
> quite yet.

I think before this lot happens you need to decide where these structures
belong. Termios and termios_locked for example could live in the tty_port
as the physical tty is incapable of having multiple sets of terminal data
at once.

Really though this looks to me like *MASSIVE* churn for now purpose.
Create a tty_struct kernel side, and use that, the needed patch is then
tiny.

so IMHO NAK

Alan

Re: [PATCH 3/9] tty: move hw_stopped flag to tty_port

[One Thousand Gnomes]

On Fri,  9 Sep 2016 17:37:04 -0500
Rob Herring  wrote:

> hw_stopped is a property of the tty port rather than the tty, so move it
> to tty_port struct and remove another dependency on tty_struct from
> drivers.
> 
> Converted with coccinelle:

This one makes sense. It's moving the object to the right place


Acked-by: Alan Cox 

Re: [PATCH 3/9] tty: move hw_stopped flag to tty_port

[One Thousand Gnomes]

On Fri,  9 Sep 2016 17:37:04 -0500
Rob Herring  wrote:

> hw_stopped is a property of the tty port rather than the tty, so move it
> to tty_port struct and remove another dependency on tty_struct from
> drivers.
> 
> Converted with coccinelle:

This one makes sense. It's moving the object to the right place


Acked-by: Alan Cox 

Re: [lkp] [tty] 761ed4a945: BUG: unable to handle kernel NULL pointer dereference at 000000000000046c

[One Thousand Gnomes]

On Fri, 9 Sep 2016 11:40:47 -0500
Rob Herring  wrote:

> On Wed, Sep 7, 2016 at 3:47 PM, Rob Herring  wrote:
> > On Tue, Sep 6, 2016 at 1:33 AM, kernel test robot  
> > wrote:  
> >>
> >> FYI, we noticed the following commit:
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> >> commit 761ed4a94582ab291aa24dcbea4e01e8936488c8 ("tty: serial_core: 
> >> convert uart_close to use tty_port_close")
> >>
> >> in testcase: boot
> >>
> >> on test machine: qemu-system-x86_64 -enable-kvm -cpu host -smp 2 -m 4G  
> >
> > Is the rootfs used in this test available to help me reproduce?  
> 
> I reproduced this now. There's a couple of ways to fix this, so I'm
> looking for some advice. The problem is tty_port->ops.shutdown()
> doesn't get called for a port marked as a console. So interrupts can
> still fire and then uart_tx_stopped() called after tty_struct becomes
> NULL. I can add a NULL check in uart_tx_stopped to fix that. There are
> possibly other spots that need a similar fix. That's going to be
> needed anyway as I remove dependencies on tty_struct. The 2nd
> possibility is just not marking the tty_port as a console. That seems
> wrong, but is what we had before this change. A third option is move
> the console handling in tty_port_shutdown into the tty_port drivers.
> That seems the wrong direction too. Thoughts?

The uart layer handles console itself so I'd suggest the quick fix for
now. The other reason for that is that if we can open/close/write to a
tty port without a tty most of the existing magic crap for consoles goes
away because a console can just open and close the tty port directly.

Alan

Re: [lkp] [tty] 761ed4a945: BUG: unable to handle kernel NULL pointer dereference at 000000000000046c

[One Thousand Gnomes]

On Fri, 9 Sep 2016 11:40:47 -0500
Rob Herring  wrote:

> On Wed, Sep 7, 2016 at 3:47 PM, Rob Herring  wrote:
> > On Tue, Sep 6, 2016 at 1:33 AM, kernel test robot  
> > wrote:  
> >>
> >> FYI, we noticed the following commit:
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> >> commit 761ed4a94582ab291aa24dcbea4e01e8936488c8 ("tty: serial_core: 
> >> convert uart_close to use tty_port_close")
> >>
> >> in testcase: boot
> >>
> >> on test machine: qemu-system-x86_64 -enable-kvm -cpu host -smp 2 -m 4G  
> >
> > Is the rootfs used in this test available to help me reproduce?  
> 
> I reproduced this now. There's a couple of ways to fix this, so I'm
> looking for some advice. The problem is tty_port->ops.shutdown()
> doesn't get called for a port marked as a console. So interrupts can
> still fire and then uart_tx_stopped() called after tty_struct becomes
> NULL. I can add a NULL check in uart_tx_stopped to fix that. There are
> possibly other spots that need a similar fix. That's going to be
> needed anyway as I remove dependencies on tty_struct. The 2nd
> possibility is just not marking the tty_port as a console. That seems
> wrong, but is what we had before this change. A third option is move
> the console handling in tty_port_shutdown into the tty_port drivers.
> That seems the wrong direction too. Thoughts?

The uart layer handles console itself so I'd suggest the quick fix for
now. The other reason for that is that if we can open/close/write to a
tty port without a tty most of the existing magic crap for consoles goes
away because a console can just open and close the tty port directly.

Alan

Re: tty: use-after-free in n_tty_receive_buf_fast

[One Thousand Gnomes]

On Sat, 3 Sep 2016 14:42:08 +0200
Dmitry Vyukov  wrote:

> Hello,
> 
> The following program causes use-after-free in n_tty_receive_buf_fast:
> 
> https://gist.githubusercontent.com/dvyukov/ac81bed0238f280ddf9067e6234cd8b0/raw/791c07ac0cdb27e2e399464d68fa0234d2aa8bd1/gistfile1.txt
>

Known bug. It's even been documented as broken since 2012, although it's
always been broken. Apparently nobody cares about fixing it although now
the tty buffers belong to the tty_port it is fixable if and when someone
dares to fix the mess that is the console locking code (because you have
to ensure the keyboard, selection and any other queue sources have to be
serialized). 

TIOCSTI is broken as well and needs to be dealt with at the same time - in
fact you can currently get a three way race between select, console input
and TIOCSTI if you really want to screw up (and you don't need root for
any of them).

Alan

Re: tty: use-after-free in n_tty_receive_buf_fast

[One Thousand Gnomes]

On Sat, 3 Sep 2016 14:42:08 +0200
Dmitry Vyukov  wrote:

> Hello,
> 
> The following program causes use-after-free in n_tty_receive_buf_fast:
> 
> https://gist.githubusercontent.com/dvyukov/ac81bed0238f280ddf9067e6234cd8b0/raw/791c07ac0cdb27e2e399464d68fa0234d2aa8bd1/gistfile1.txt
>

Known bug. It's even been documented as broken since 2012, although it's
always been broken. Apparently nobody cares about fixing it although now
the tty buffers belong to the tty_port it is fixable if and when someone
dares to fix the mess that is the console locking code (because you have
to ensure the keyboard, selection and any other queue sources have to be
serialized). 

TIOCSTI is broken as well and needs to be dealt with at the same time - in
fact you can currently get a three way race between select, console input
and TIOCSTI if you really want to screw up (and you don't need root for
any of them).

Alan

Re: 6pack: stack-out-of-bounds in sixpack_receive_buf

[One Thousand Gnomes]

> different runs). Looking at code, the following looks suspicious -- we
> limit copy by 512 bytes, but use the original count which can be
> larger than 512:
> 
> static void sixpack_receive_buf(struct tty_struct *tty,
> const unsigned char *cp, char *fp, int count)
> {
> unsigned char buf[512];
> 
> memcpy(buf, cp, count < sizeof(buf) ? count : sizeof(buf));
> 
> sixpack_decode(sp, buf, count1);
> 
> 
> On commit 0f98f121e1670eaa2a2fbb675e07d6ba7f0e146f of linux-next.

With the sane tty locking we now have I believe the following is safe as
we consume the bytes and move them into the decoded buffer before
returning.

diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
index 5a1e985..470b3dc 100644
--- a/drivers/net/hamradio/6pack.c
+++ b/drivers/net/hamradio/6pack.c
@@ -127,7 +127,7 @@ struct sixpack {
 
 #define AX25_6PACK_HEADER_LEN 0
 
-static void sixpack_decode(struct sixpack *, unsigned char[], int);
+static void sixpack_decode(struct sixpack *, const unsigned char[], int);
 static int encode_sixpack(unsigned char *, unsigned char *, int, unsigned 
char);
 
 /*
@@ -428,7 +428,7 @@ out:
 
 /*
  * Handle the 'receiver data ready' interrupt.
- * This function is called by the 'tty_io' module in the kernel when
+ * This function is called by the tty module in the kernel when
  * a block of 6pack data has been received, which can now be decapsulated
  * and sent on to some IP layer for further processing.
  */
@@ -436,7 +436,6 @@ static void sixpack_receive_buf(struct tty_struct *tty,
const unsigned char *cp, char *fp, int count)
 {
struct sixpack *sp;
-   unsigned char buf[512];
int count1;
 
if (!count)
@@ -446,10 +445,7 @@ static void sixpack_receive_buf(struct tty_struct *tty,
if (!sp)
return;
 
-   memcpy(buf, cp, count < sizeof(buf) ? count : sizeof(buf));
-
/* Read the characters out of the buffer */
-
count1 = count;
while (count) {
count--;
@@ -459,7 +455,7 @@ static void sixpack_receive_buf(struct tty_struct *tty,
continue;
}
}
-   sixpack_decode(sp, buf, count1);
+   sixpack_decode(sp, cp, count1);
 
sp_put(sp);
tty_unthrottle(tty);
@@ -992,7 +988,7 @@ static void decode_std_command(struct sixpack *sp, unsigned 
char cmd)
 /* decode a 6pack packet */
 
 static void
-sixpack_decode(struct sixpack *sp, unsigned char *pre_rbuff, int count)
+sixpack_decode(struct sixpack *sp, const unsigned char *pre_rbuff, int count)
 {
unsigned char inbyte;
int count1;

Re: 6pack: stack-out-of-bounds in sixpack_receive_buf

[One Thousand Gnomes]

> different runs). Looking at code, the following looks suspicious -- we
> limit copy by 512 bytes, but use the original count which can be
> larger than 512:
> 
> static void sixpack_receive_buf(struct tty_struct *tty,
> const unsigned char *cp, char *fp, int count)
> {
> unsigned char buf[512];
> 
> memcpy(buf, cp, count < sizeof(buf) ? count : sizeof(buf));
> 
> sixpack_decode(sp, buf, count1);
> 
> 
> On commit 0f98f121e1670eaa2a2fbb675e07d6ba7f0e146f of linux-next.

With the sane tty locking we now have I believe the following is safe as
we consume the bytes and move them into the decoded buffer before
returning.

diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
index 5a1e985..470b3dc 100644
--- a/drivers/net/hamradio/6pack.c
+++ b/drivers/net/hamradio/6pack.c
@@ -127,7 +127,7 @@ struct sixpack {
 
 #define AX25_6PACK_HEADER_LEN 0
 
-static void sixpack_decode(struct sixpack *, unsigned char[], int);
+static void sixpack_decode(struct sixpack *, const unsigned char[], int);
 static int encode_sixpack(unsigned char *, unsigned char *, int, unsigned 
char);
 
 /*
@@ -428,7 +428,7 @@ out:
 
 /*
  * Handle the 'receiver data ready' interrupt.
- * This function is called by the 'tty_io' module in the kernel when
+ * This function is called by the tty module in the kernel when
  * a block of 6pack data has been received, which can now be decapsulated
  * and sent on to some IP layer for further processing.
  */
@@ -436,7 +436,6 @@ static void sixpack_receive_buf(struct tty_struct *tty,
const unsigned char *cp, char *fp, int count)
 {
struct sixpack *sp;
-   unsigned char buf[512];
int count1;
 
if (!count)
@@ -446,10 +445,7 @@ static void sixpack_receive_buf(struct tty_struct *tty,
if (!sp)
return;
 
-   memcpy(buf, cp, count < sizeof(buf) ? count : sizeof(buf));
-
/* Read the characters out of the buffer */
-
count1 = count;
while (count) {
count--;
@@ -459,7 +455,7 @@ static void sixpack_receive_buf(struct tty_struct *tty,
continue;
}
}
-   sixpack_decode(sp, buf, count1);
+   sixpack_decode(sp, cp, count1);
 
sp_put(sp);
tty_unthrottle(tty);
@@ -992,7 +988,7 @@ static void decode_std_command(struct sixpack *sp, unsigned 
char cmd)
 /* decode a 6pack packet */
 
 static void
-sixpack_decode(struct sixpack *sp, unsigned char *pre_rbuff, int count)
+sixpack_decode(struct sixpack *sp, const unsigned char *pre_rbuff, int count)
 {
unsigned char inbyte;
int count1;

Re: 6pack: stack-out-of-bounds in sixpack_receive_buf

[One Thousand Gnomes]

On Sat, 3 Sep 2016 15:38:08 +0200
Dmitry Vyukov  wrote:

> Hello,
> 
> While running syzkaller fuzzer I've got the following report:
> 
> BUG: KASAN: stack-out-of-bounds in sixpack_receive_buf+0xf8a/0x1450 at
> addr 880037fbf850
> Read of size 1 by task syz-executor/6759
> page:eadfefc0 count:0 mapcount:0 mapping:  (null) index:0x0
> flags: 0x1fffc00()
> page dumped because: kasan: bad access detected
> CPU: 3 PID: 6759 Comm: syz-executor Not tainted 4.8.0-rc3-next-20160825+ #8
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>  886b6fe0 880037fbf520 82db38d9 37fbf5b0
>  fbfff10d6dfc 880037fbf5b0 880037fbf850 880037fbf850
>  880037d3f180 dc00 880037fbf5a0 8180a383
> Call Trace:
>  [] __asan_report_load1_noabort+0x3e/0x40
> mm/kasan/report.c:319
>  [< inline >] sixpack_decode drivers/net/hamradio/6pack.c:1001
>  [] sixpack_receive_buf+0xf8a/0x1450
> drivers/net/hamradio/6pack.c:462
>  [] tty_ldisc_receive_buf+0x168/0x1b0
> drivers/tty/tty_buffer.c:433
>  [] paste_selection+0x27e/0x3e0 
> drivers/tty/vt/selection.c:363
>  [] tioclinux+0x126/0x410 drivers/tty/vt/vt.c:2683
>  [] vt_ioctl+0x13ef/0x2910 drivers/tty/vt/vt_ioctl.c:365
>  [] tty_ioctl+0x69d/0x21e0 drivers/tty/tty_io.c:2983
>  [< inline >] vfs_ioctl fs/ioctl.c:43
>  [] do_vfs_ioctl+0x18c/0x1080 fs/ioctl.c:675
>  [< inline >] SYSC_ioctl fs/ioctl.c:690
>  [] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:681
>  [] entry_SYSCALL_64_fastpath+0x23/0xc1
> Memory state around the buggy address:
>  880037fbf700: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  880037fbf780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >880037fbf800: 00 00 00 00 00 00 00 00 00 00 f3 f3 f3 f3 00 00  
>  ^
>  880037fbf880: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  880037fbf900: 00 00 00 00 f1 f1 f1 f1 00 f4 f4 f4 f2 f2 f2 f2
> ==
> 
> 
> It is then followed by similar reports that access subsequent stack bytes.
> Unfortunately I can't reproduce it (though, I got 6 similar crashes in
> different runs). Looking at code, the following looks suspicious -- we
> limit copy by 512 bytes, but use the original count which can be
> larger than 512:
> 
> static void sixpack_receive_buf(struct tty_struct *tty,
> const unsigned char *cp, char *fp, int count)
> {
> unsigned char buf[512];
> 
> memcpy(buf, cp, count < sizeof(buf) ? count : sizeof(buf));
> 
> sixpack_decode(sp, buf, count1);
> 

Your suspicion is correct.

Alan

Re: 6pack: stack-out-of-bounds in sixpack_receive_buf

[One Thousand Gnomes]

On Sat, 3 Sep 2016 15:38:08 +0200
Dmitry Vyukov  wrote:

> Hello,
> 
> While running syzkaller fuzzer I've got the following report:
> 
> BUG: KASAN: stack-out-of-bounds in sixpack_receive_buf+0xf8a/0x1450 at
> addr 880037fbf850
> Read of size 1 by task syz-executor/6759
> page:eadfefc0 count:0 mapcount:0 mapping:  (null) index:0x0
> flags: 0x1fffc00()
> page dumped because: kasan: bad access detected
> CPU: 3 PID: 6759 Comm: syz-executor Not tainted 4.8.0-rc3-next-20160825+ #8
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>  886b6fe0 880037fbf520 82db38d9 37fbf5b0
>  fbfff10d6dfc 880037fbf5b0 880037fbf850 880037fbf850
>  880037d3f180 dc00 880037fbf5a0 8180a383
> Call Trace:
>  [] __asan_report_load1_noabort+0x3e/0x40
> mm/kasan/report.c:319
>  [< inline >] sixpack_decode drivers/net/hamradio/6pack.c:1001
>  [] sixpack_receive_buf+0xf8a/0x1450
> drivers/net/hamradio/6pack.c:462
>  [] tty_ldisc_receive_buf+0x168/0x1b0
> drivers/tty/tty_buffer.c:433
>  [] paste_selection+0x27e/0x3e0 
> drivers/tty/vt/selection.c:363
>  [] tioclinux+0x126/0x410 drivers/tty/vt/vt.c:2683
>  [] vt_ioctl+0x13ef/0x2910 drivers/tty/vt/vt_ioctl.c:365
>  [] tty_ioctl+0x69d/0x21e0 drivers/tty/tty_io.c:2983
>  [< inline >] vfs_ioctl fs/ioctl.c:43
>  [] do_vfs_ioctl+0x18c/0x1080 fs/ioctl.c:675
>  [< inline >] SYSC_ioctl fs/ioctl.c:690
>  [] SyS_ioctl+0x8f/0xc0 fs/ioctl.c:681
>  [] entry_SYSCALL_64_fastpath+0x23/0xc1
> Memory state around the buggy address:
>  880037fbf700: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  880037fbf780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >880037fbf800: 00 00 00 00 00 00 00 00 00 00 f3 f3 f3 f3 00 00  
>  ^
>  880037fbf880: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  880037fbf900: 00 00 00 00 f1 f1 f1 f1 00 f4 f4 f4 f2 f2 f2 f2
> ==
> 
> 
> It is then followed by similar reports that access subsequent stack bytes.
> Unfortunately I can't reproduce it (though, I got 6 similar crashes in
> different runs). Looking at code, the following looks suspicious -- we
> limit copy by 512 bytes, but use the original count which can be
> larger than 512:
> 
> static void sixpack_receive_buf(struct tty_struct *tty,
> const unsigned char *cp, char *fp, int count)
> {
> unsigned char buf[512];
> 
> memcpy(buf, cp, count < sizeof(buf) ? count : sizeof(buf));
> 
> sixpack_decode(sp, buf, count1);
> 

Your suspicion is correct.

Alan

Re: tty: sleeping function in invalid in context do_con_write and deadlock in gsm_control_retransmit

[One Thousand Gnomes]

On Sat, 3 Sep 2016 13:07:27 +0200
Dmitry Vyukov  wrote:

> Hello,
> 
> While running syzkaller fuzzer on
> 0f98f121e1670eaa2a2fbb675e07d6ba7f0e146f of linux-next, I've for the
> following splash. Note there are 2 separate bugs (but maybe related):
> 

There are a couple of known cases where strange ldiscs plus console
breaks because the console locking is still completely fubar. The gsm one
is known.

Alan

Re: tty: sleeping function in invalid in context do_con_write and deadlock in gsm_control_retransmit

[One Thousand Gnomes]

On Sat, 3 Sep 2016 13:07:27 +0200
Dmitry Vyukov  wrote:

> Hello,
> 
> While running syzkaller fuzzer on
> 0f98f121e1670eaa2a2fbb675e07d6ba7f0e146f of linux-next, I've for the
> following splash. Note there are 2 separate bugs (but maybe related):
> 

There are a couple of known cases where strange ldiscs plus console
breaks because the console locking is still completely fubar. The gsm one
is known.

Alan

Re: [PATCH] binfmt_misc: allow selecting the interpreter based on xattr keywords

[One Thousand Gnomes]

On Fri, 26 Aug 2016 17:26:18 -0400
James Bottomley <james.bottom...@hansenpartnership.com> wrote:

> On Fri, 2016-08-26 at 22:12 +0100, One Thousand Gnomes wrote:
> > > A non-security use case would be to run the binary (without 
> > > modification) with a different ELF interpreter (assuming this 
> > > allows to override binfmt_elf, but self-sandboxing would need that 
> > > as well).  This would make it easier to use older or newer libcs 
> > > for select binaries on the system.  Right now, one has to write 
> > > wrappers for that, and the explicit dynamic linker invocation is 
> > > not completely transparent to the application.  
> > 
> > If it gets in I'll be using it to label CP/M COM files so that they 
> > can be auto-run nicely when crossbuilding stuff in part with the 
> > original tools but a modern build environment 8)
> > 
> > Sandboxing is an obvious use but there are more bizarre ones such as
> > marking a file system image to get auto-run under a virtual machine 
> > or make containers fire up as if they were commands.  
> 
> So I asked previously but didn't get an answer.  If this is useful for
> sandboxing and being in the sandbox depends on the xattr value,
> shouldn't it be in one of the privileged xattr namespaces, not the
> user. one?

IMHO no 

- because it's not giving additional rights, it is taking rights away
  voluntarily
- because as a user I can simply cp the file to get an unsandboxed version

If it was a setuid like bit then yes it would matter.

Alan

Re: [PATCH] binfmt_misc: allow selecting the interpreter based on xattr keywords

[One Thousand Gnomes]

On Fri, 26 Aug 2016 17:26:18 -0400
James Bottomley  wrote:

> On Fri, 2016-08-26 at 22:12 +0100, One Thousand Gnomes wrote:
> > > A non-security use case would be to run the binary (without 
> > > modification) with a different ELF interpreter (assuming this 
> > > allows to override binfmt_elf, but self-sandboxing would need that 
> > > as well).  This would make it easier to use older or newer libcs 
> > > for select binaries on the system.  Right now, one has to write 
> > > wrappers for that, and the explicit dynamic linker invocation is 
> > > not completely transparent to the application.  
> > 
> > If it gets in I'll be using it to label CP/M COM files so that they 
> > can be auto-run nicely when crossbuilding stuff in part with the 
> > original tools but a modern build environment 8)
> > 
> > Sandboxing is an obvious use but there are more bizarre ones such as
> > marking a file system image to get auto-run under a virtual machine 
> > or make containers fire up as if they were commands.  
> 
> So I asked previously but didn't get an answer.  If this is useful for
> sandboxing and being in the sandbox depends on the xattr value,
> shouldn't it be in one of the privileged xattr namespaces, not the
> user. one?

IMHO no 

- because it's not giving additional rights, it is taking rights away
  voluntarily
- because as a user I can simply cp the file to get an unsandboxed version

If it was a setuid like bit then yes it would matter.

Alan

Re: [PATCH] binfmt_misc: allow selecting the interpreter based on xattr keywords

[One Thousand Gnomes]

> A non-security use case would be to run the binary (without 
> modification) with a different ELF interpreter (assuming this allows to 
> override binfmt_elf, but self-sandboxing would need that as well).  This 
> would make it easier to use older or newer libcs for select binaries on 
> the system.  Right now, one has to write wrappers for that, and the 
> explicit dynamic linker invocation is not completely transparent to the 
> application.

If it gets in I'll be using it to label CP/M COM files so that they can
be auto-run nicely when crossbuilding stuff in part with the original
tools but a modern build environment 8)

Sandboxing is an obvious use but there are more bizarre ones such as
marking a file system image to get auto-run under a virtual machine or
make containers fire up as if they were commands.

It's effectively also the long missing but much needed "this document
belongs to this app" meta data that MacOS and the like have had for
decades.

Alan

Re: [PATCH] binfmt_misc: allow selecting the interpreter based on xattr keywords

[One Thousand Gnomes]

> A non-security use case would be to run the binary (without 
> modification) with a different ELF interpreter (assuming this allows to 
> override binfmt_elf, but self-sandboxing would need that as well).  This 
> would make it easier to use older or newer libcs for select binaries on 
> the system.  Right now, one has to write wrappers for that, and the 
> explicit dynamic linker invocation is not completely transparent to the 
> application.

If it gets in I'll be using it to label CP/M COM files so that they can
be auto-run nicely when crossbuilding stuff in part with the original
tools but a modern build environment 8)

Sandboxing is an obvious use but there are more bizarre ones such as
marking a file system image to get auto-run under a virtual machine or
make containers fire up as if they were commands.

It's effectively also the long missing but much needed "this document
belongs to this app" meta data that MacOS and the like have had for
decades.

Alan

Re: [RFC PATCH 0/3] UART slave device bus

[One Thousand Gnomes]

> Either we'd have to call tty_port->rx a character at a time or
> implement some temporary buffer. I don't think we want to call things
> like BT receive code a byte at a time. This needs to be a layer
> higher. flush_to_ldisc either needs to be duplicated to handle
> tty_port->rx or generalized to call either tty_port->rx or ldisc
> receive_buf. I'm not sure what to do about ldisc ref counting in the
> latter case.

You already have the buffer

What I was trying to suggest was that instead of


chars->buffer
flush
workqueue
loop pushing data into the ldisc


You can also do

chars->buffer
flush
workqueue
->rx()  [where flush_to_ldisc is just one implementation of ->rx]

For byte by byte ports it really makes no difference (except you would be
able to do processing without an ldisc), but for DMA devices it would
give us a faster path for processing since the DMA completion event can
simply fire a buffer a characters directly at the protocol handler where
there are not latency concerns (ie it starts the new DMA and directly
involves ->rx())

Alan

Re: [RFC PATCH 0/3] UART slave device bus

[One Thousand Gnomes]

> Either we'd have to call tty_port->rx a character at a time or
> implement some temporary buffer. I don't think we want to call things
> like BT receive code a byte at a time. This needs to be a layer
> higher. flush_to_ldisc either needs to be duplicated to handle
> tty_port->rx or generalized to call either tty_port->rx or ldisc
> receive_buf. I'm not sure what to do about ldisc ref counting in the
> latter case.

You already have the buffer

What I was trying to suggest was that instead of


chars->buffer
flush
workqueue
loop pushing data into the ldisc


You can also do

chars->buffer
flush
workqueue
->rx()  [where flush_to_ldisc is just one implementation of ->rx]

For byte by byte ports it really makes no difference (except you would be
able to do processing without an ldisc), but for DMA devices it would
give us a faster path for processing since the DMA completion event can
simply fire a buffer a characters directly at the protocol handler where
there are not latency concerns (ie it starts the new DMA and directly
involves ->rx())

Alan

Re: CVE-2014-9900 fix is not upstream

[One Thousand Gnomes]

> > I see someone did request it 2 years ago:
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63479  
> 
> I don't think this is sufficient. Basically if you write one field in a
> struct after a memset again, the compiler is allowed by the standard to
> write padding bytes again, causing them to be undefined.

The question is simply what gcc actually does. The rest is C language
lawyering and since the kernel isn't written to the C language spec but
to gcc only gcc matters.

Alan

Re: CVE-2014-9900 fix is not upstream

[One Thousand Gnomes]

> > I see someone did request it 2 years ago:
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63479  
> 
> I don't think this is sufficient. Basically if you write one field in a
> struct after a memset again, the compiler is allowed by the standard to
> write padding bytes again, causing them to be undefined.

The question is simply what gcc actually does. The rest is C language
lawyering and since the kernel isn't written to the C language spec but
to gcc only gcc matters.

Alan

Re: [RFC PATCH 0/3] UART slave device bus

[One Thousand Gnomes]

> So you mean if I do "hciconfig hci0 down", then the uart-bus should
> "down" the tty and only on "hciconfig hci0 up" it should "up" the
> tty? I would expect a uart-bus slave-device takes control of the
> device ("up" it) on probe. It's hardwired anyway.

Today you can switch stacks at runtime, you can switch between the kernel
stack and debug tools at runtime. Breaking that is a regression.

> Also what should happen if old userspace use hciattach while
> uart-bus slave-device doesn't have control over it? Do you

You would either use the old hciattach in which case you wouldn't be able
to manage it via a new API while attached, or the new API in which case
you wouldn't be able to manage it via the old interface while it was
being used directly.

> Or do you suggest to register hci1 and one cannot use hci0? I guess
> this breaks even more devices, as the device number changes.

Device numbers are dynamic anyway. Plug a USB adapter in and if it beats
your onboard adapter to registration then the order changes.

> So yes, from your point of view there is a regression, just because
> it's working automatically. So let's just not convert existing boards
> with working hciattach based bluetooth devices. New devices can use

>From a distribution point of view that would be a nightmare.

> the uart-bus, as it's not a regression for them and Nokia N series
> can also do it, since they have no working bluetooth at all at the
> moment.

The Nokia N series is a weird corner case.

> > In many cases you'll also still need the tty interface to do
> > things like firmware upgrades.  
> 
> I would expect the uart-slave driver to know how to do firmware
> updates. Actually most bluetooth chips are initialized by uploading
> a firmware to them.

Usually no - you don't want a ton of kernel code for flashing adapters
when they have built in firmware (similar issue for 3G modems)

> And there are definitely uart drivers not caring about having a tty
> device. Nokia's vendor driver for their bluetooth protocol contains
> a custom omap-serial driver combined with the actual bluetooth
> driver. There is nothing related to the tty framework. I think the
> same would work for the other hardwired bluetooth chips perfectly
> fine.

That means having two different omap serial drivers to maintain which is
not ideal.

To me there are four different things

1. bluetooth devices "just work". That can be user space (eg it seems to
just work on my Fedora boxes and bluetooth enumeration is being done via
user space, or may be via kernel enumeration, or a mix. PPP is an
existing example of this - serial port PPP is an ldisc but ports that are
not UART like speak directly to the PPP layer as network adapters.

2. Sideband controls and power management, where we need to give the
tty_port a child device and power it up/down properly and have the
tty_port hooks to do so based upon the ldisc protocol state machine when
talking stuff like NMEA or HCI.

3. The special case UART power saving features/hacks like GPIO snooping,
again with the right hooks

4. Whether it's useful to be able to create a tty device in kernel and
attach that to stuff with no userspace involved.

All are independent.

Alan

Re: [RFC PATCH 0/3] UART slave device bus

[One Thousand Gnomes]

> So you mean if I do "hciconfig hci0 down", then the uart-bus should
> "down" the tty and only on "hciconfig hci0 up" it should "up" the
> tty? I would expect a uart-bus slave-device takes control of the
> device ("up" it) on probe. It's hardwired anyway.

Today you can switch stacks at runtime, you can switch between the kernel
stack and debug tools at runtime. Breaking that is a regression.

> Also what should happen if old userspace use hciattach while
> uart-bus slave-device doesn't have control over it? Do you

You would either use the old hciattach in which case you wouldn't be able
to manage it via a new API while attached, or the new API in which case
you wouldn't be able to manage it via the old interface while it was
being used directly.

> Or do you suggest to register hci1 and one cannot use hci0? I guess
> this breaks even more devices, as the device number changes.

Device numbers are dynamic anyway. Plug a USB adapter in and if it beats
your onboard adapter to registration then the order changes.

> So yes, from your point of view there is a regression, just because
> it's working automatically. So let's just not convert existing boards
> with working hciattach based bluetooth devices. New devices can use

>From a distribution point of view that would be a nightmare.

> the uart-bus, as it's not a regression for them and Nokia N series
> can also do it, since they have no working bluetooth at all at the
> moment.

The Nokia N series is a weird corner case.

> > In many cases you'll also still need the tty interface to do
> > things like firmware upgrades.  
> 
> I would expect the uart-slave driver to know how to do firmware
> updates. Actually most bluetooth chips are initialized by uploading
> a firmware to them.

Usually no - you don't want a ton of kernel code for flashing adapters
when they have built in firmware (similar issue for 3G modems)

> And there are definitely uart drivers not caring about having a tty
> device. Nokia's vendor driver for their bluetooth protocol contains
> a custom omap-serial driver combined with the actual bluetooth
> driver. There is nothing related to the tty framework. I think the
> same would work for the other hardwired bluetooth chips perfectly
> fine.

That means having two different omap serial drivers to maintain which is
not ideal.

To me there are four different things

1. bluetooth devices "just work". That can be user space (eg it seems to
just work on my Fedora boxes and bluetooth enumeration is being done via
user space, or may be via kernel enumeration, or a mix. PPP is an
existing example of this - serial port PPP is an ldisc but ports that are
not UART like speak directly to the PPP layer as network adapters.

2. Sideband controls and power management, where we need to give the
tty_port a child device and power it up/down properly and have the
tty_port hooks to do so based upon the ldisc protocol state machine when
talking stuff like NMEA or HCI.

3. The special case UART power saving features/hacks like GPIO snooping,
again with the right hooks

4. Whether it's useful to be able to create a tty device in kernel and
attach that to stuff with no userspace involved.

All are independent.

Alan

Re: [PATCH 1/2] tty: serial_core: convert uart_open to use tty_port_open

[One Thousand Gnomes]

On Mon, 22 Aug 2016 17:39:09 -0500
Rob Herring  wrote:

> tty_port_open handles much of the common parts of tty opening. Convert
> uart_open to use it and move the serial_core specific parts into
> tty_port.activate function. This will be needed to use tty_port functions
> directly from in kernel clients.
> 
> The tricky part is uart_port_startup can return positive values to allow
> setserial to configure the port. We now return the positive value to
> tty_port_open so that the tty is not marked as initialized and then set the
> return value in uart_open to 0.
> 
> Cc: Alan Cox 
> Cc: Peter Hurley 
> Signed-off-by: Rob Herring 
> ---
> In looking at using tty_port for uart slaves, this is the first thing I 
> hit. serial_core doesn't even use the 2 functions that already exist for 
> tty_port and are needed.
> 
> I've tested this on QEMU with pl011 and 8250 UARTs using setserial, but 
> I'm not sure what are all the tricky cases to handle.
> 
> Greg, Don't apply these without comment from Alan and Peter.

Originally I didn't convert them because the lockign was incompatible.
That looks to have been fixed by other changes. This is definitely the
right direction irrespective of the uart slave discussion.

Acked-by: Alan Cox 

Re: [PATCH 1/2] tty: serial_core: convert uart_open to use tty_port_open

[One Thousand Gnomes]

On Mon, 22 Aug 2016 17:39:09 -0500
Rob Herring  wrote:

> tty_port_open handles much of the common parts of tty opening. Convert
> uart_open to use it and move the serial_core specific parts into
> tty_port.activate function. This will be needed to use tty_port functions
> directly from in kernel clients.
> 
> The tricky part is uart_port_startup can return positive values to allow
> setserial to configure the port. We now return the positive value to
> tty_port_open so that the tty is not marked as initialized and then set the
> return value in uart_open to 0.
> 
> Cc: Alan Cox 
> Cc: Peter Hurley 
> Signed-off-by: Rob Herring 
> ---
> In looking at using tty_port for uart slaves, this is the first thing I 
> hit. serial_core doesn't even use the 2 functions that already exist for 
> tty_port and are needed.
> 
> I've tested this on QEMU with pl011 and 8250 UARTs using setserial, but 
> I'm not sure what are all the tricky cases to handle.
> 
> Greg, Don't apply these without comment from Alan and Peter.

Originally I didn't convert them because the lockign was incompatible.
That looks to have been fixed by other changes. This is definitely the
right direction irrespective of the uart slave discussion.

Acked-by: Alan Cox 

Re: [REGRESSION] Select hang with zero sized UDP packets

[One Thousand Gnomes]

On Wed, 24 Aug 2016 11:22:09 +0300
"Dan Akunis"  wrote:

> When select wakes up on a UDP socket, user is expecting to get data. Getting 
> 0 from recvfrom() or whatever read function she uses, is a wrong attitude.
> I agree with David.
> 
> The unit test that expects select to wake up is wrong and should be changed.

The unit test is correct.

The behaviour of a 0 byte frame is actually well established and a 0 byte
data frame is a meaningful message is several protocols. It is distinct
from no pending data because that would report EWOULDBLOCK. It's more fun
with regard to EOF but UDP has no EOF semantics. If you want to
understand how to handle zero length datagrams in a connection oriented
protocol the old DECnet documentation covers it in all its pain 8)

Alan

Re: [REGRESSION] Select hang with zero sized UDP packets

[One Thousand Gnomes]

On Wed, 24 Aug 2016 11:22:09 +0300
"Dan Akunis"  wrote:

> When select wakes up on a UDP socket, user is expecting to get data. Getting 
> 0 from recvfrom() or whatever read function she uses, is a wrong attitude.
> I agree with David.
> 
> The unit test that expects select to wake up is wrong and should be changed.

The unit test is correct.

The behaviour of a 0 byte frame is actually well established and a 0 byte
data frame is a meaningful message is several protocols. It is distinct
from no pending data because that would report EWOULDBLOCK. It's more fun
with regard to EOF but UDP has no EOF semantics. If you want to
understand how to handle zero length datagrams in a connection oriented
protocol the old DECnet documentation covers it in all its pain 8)

Alan

Re: [RFC PATCH 0/3] UART slave device bus

[One Thousand Gnomes]

> > That would still be a regression. Not everyone even uses the kernel
> > bluetooth stack. It would only return EBUSY if you had done an "up"
> > on it via the direct bluetooth stack.  
> 
> So it returns EBUSY when uart-bus is used. Since uart-bus is about
> hardwired devices that's basically always.

That would only be when the bluetooth port in question was active via the
hardwired interface - which is not always. You choose to turn on/off
bluetooth interfaces. If you boot with an older user space you'd use
hciattach instead.

In many cases you'll also still need the tty interface to do things like
firmware upgrades.

Alan

Re: [RFC PATCH 0/3] UART slave device bus

[One Thousand Gnomes]

> > That would still be a regression. Not everyone even uses the kernel
> > bluetooth stack. It would only return EBUSY if you had done an "up"
> > on it via the direct bluetooth stack.  
> 
> So it returns EBUSY when uart-bus is used. Since uart-bus is about
> hardwired devices that's basically always.

That would only be when the bluetooth port in question was active via the
hardwired interface - which is not always. You choose to turn on/off
bluetooth interfaces. If you boot with an older user space you'd use
hciattach instead.

In many cases you'll also still need the tty interface to do things like
firmware upgrades.

Alan