Re: Crash when specifying non-existent serial port in speakup / tty_kopen

[Matthias Reichl <hias@horus.com>]

Hi Samuel,

On Wed, Nov 04, 2020 at 09:13:23PM +0100, Samuel Thibault wrote:
> Hello,
> 
> Matthias Reichl, le mer. 04 nov. 2020 15:57:37 +0100, a ecrit:
> > I initially noticed this oops on x86_64 running kernel 5.4.59 when
> > I accidentally mistyped "ttyS0" as "ttyS9":
> > 
> > modprobe speakup_dummy dev=ttyS9
> 
> > [   49.978481] tty_init_dev: ttyS driver does not set tty->port. This would crash the kernel. Fix the driver!
> 
> This looks like only a warning, did it actually crash?

Yes, scroll down a bit, the null pointer oops followed almost
immediately after that

[   49.979043] BUG: kernel NULL pointer dereference, address: 0000000000000090

> > the missing tty->port is quite fatal.
> 
> It is fatal for module insertion yes (EINVAL) but IIRC that should be
> getting handled properly, making modprobe return the error?

When I did that on my desktop the tty system was pretty screwed. Mouse still
worked in X but no keyboard input possible.

> > It looks like spk_ttyio or tty_dev_name_to_number() / tty_kopen()
> > should perform some additional validation,
> 
> spk_ttyio_initialise_ldisc only has a dev_t so can't do much beyond
> calling tty_kopen.
> 
> tty_kopen is getting the index from the tty_lookup_driver call (actually
> get_tty_driver which uses p->minor_start and p->num) and passes it to
> tty_driver_lookup_tty. Perhaps in addition of p->num the driver should
> have another field to set, that tty_init_dev could use to reject with
> ENODEV indexes beyond what the driver actually provides?

It might be a bit more involved than a simple max port check, think
about hotplug (I have 16C950 ExpressCard devices I occansionally use
on one of my laptops) so there may be holes in the allocation numbers.
Not sure how/where to best solve this.

> > I couldn't make the kernel warn/crash yet by specifying non-existent
> > ttyUSB ports yet though.
> 
> That's probably because in the ttyUSB case the device allocation is
> dynamic and made exactly according to the number of actual devices,
> while for ttyS* there is a large overcommit of minor values.

Yes, that sounds reasonable (haven't looked at ttyUSB details, only
checked serial core devices yet).

so long,

Hias