Re: [musl] making termios BOTHER speeds work

[Jeffrey Walton <noloader@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5769 bytes --]

On Thu, Apr 11, 2024 at 1:30 PM Rich Felker <dalias@libc.org> wrote:

> On Thu, Apr 11, 2024 at 12:55:56PM -0400, Rich Felker wrote:
> > On Thu, Apr 11, 2024 at 10:24:56AM -0400, Rich Felker wrote:
> > > Since it's come up again, I'm looking at what it would take to get
> > > support for custom baud rates in termios working. This topic is
> > > something of a mess, as it involves discrepancies between our termios
> > > structure and the kernel termios/termios2 structures.
> > >
> > > Szabolcs Nagy did some of the initial research on the mismatched
> > > definitions in 2016: https://www.openwall.com/lists/musl/2016/04/26/3
> > >
> > > Basically, it looks like what happened was that we tried to match the
> > > glibc generic ABI (an early goal of lots of stuff in musl) as long as
> > > it lined up with the kernel termios (not termios2) ioctl structure,
> > > but deviated when it wouldn't (powerpc had c_line and c_cc order
> > > flipped and we followed kernel on that), and didn't do glibc
> > > arch-specific mistakes (like mips omitting the __c_[io]speed fields).
> > >
> > > If we had used the kernel value of NCCS everywhere, rather than the
> > > inflated glibc value of 32, we could add BOTHER support just by
> > > attempting TCSETS2 using the caller's termios structure, and only
> > > falling back if it doesn't work. In theory we *could* change to do
> > > this now. The __c_[io]speed members are not in the public namespace,
> > > and NCCS could be reduced to accommodate them as long as the overall
> > > struct size was preserved. This might be ~ugly~ for programs built
> > > with the old NCCS of 32, which might copy c_cc past its new end, but
> > > they'd just be moving stuff to/from the reserved speed fields they
> > > couldn't yet be using. The worst part about this seems to be that we'd
> > > be introducing more arch-specific versions of bits/termios.h, since
> > > the "generic" definition we have now actually has different layout
> > > depending on the arch's alignment requirements. I think this only
> > > affects m68k (where it's 2 rather than 4 byte alignment for int), so
> > > maybe it's not a big deal to add just one.
> > >
> > > The alternative is to only use the caller-provided termios in-place in
> > > the case where we can get by without termios2 interfaces: that is,
> > > when either BOTHER is not set (classic POSIX baud flags), or TCSETS2
> > > is not defined (plain termios already supports BOTHER for this arch).
> > > Otherwise, translate to a kernel termios2 form, which really requires
> > > nothing other than knowing an arch-defined offset for the speed
> > > fields.
> > >
> > > For going the other direction (tcgetattr) it's even easier: we're
> > > allowed to clobber the caller buffer, so just try TCGETS2 and move the
> > > speeds from their kernel offset into the libc member offsset.
> > >
> > > I think this second approach is probably better, but I'm open to
> > > reasons why it might not be.
> >
> > One thing I hadn't even considered yet is how the application is
> > expected to set custom speeds. We don't expose BOTHER, and while we
> > could expose it and put the c_[io]speed members in the public
> > namespace for direct access, it's not clear that this is the right way
> > to do it.
> >
> > glibc's approach seems to be having cfset[io]speed accept values other
> > than the symbolic B constants, which POSIX allows and mentions in the
> > RATIONALE:
> >
> >     There is nothing to prevent an implementation accepting as an
> >     extension a number (such as 126), and since the encoding of the
> >     Bxxx symbols is not specified, this can be done to avoid
> >     introducing ambiguity.
> >
> >
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/cfgetispeed.html
> >
> > This seems like it's the better approach. It does have values 0-15 and
> > 4096-4111 as impossible-to-set because they overlap with B constants,
> > but these are not useful speeds.
>
> OK, no, it doesn't. Only the nonstandard cfsetspeed on glibc accepts
> actual numbers, and applies them to both input and output. The
> standard cfset[io]speed functions only accept the symbolic B
> constants. And... they seem to be storing symbolic B constants in the
> c_[io]speed members, which seems wrong. >_<
>
> > Of course it might be useful to look at what applications expect to be
> > able to do.
>
> Thus, applications using the glibc API here need BOTHER to be defined
> and need to directly access c_[io]speed members.
>
> This seems like an ugly leak of implementation details, but I'm not
> sure whether it would be useful to have API-incompatible support for
> custom bauds.
>

I have never encountered a need for a custom baud rate due to standardized
UART chips. There are probably some edge cases out there. I'd like to hear
about them.

Reading a baud rate from a config file that can be modified by a user
introduces tainted inputs. I clamp the speed to a B-constant to cleanse
mistakes and malicious inputs:

unsigned int term_clamp_speed(unsigned int speed)
{
#if defined(B4000000)
    if (speed >= 4000000)
        return 4000000;
    else
#endif
#if defined(B3500000)
    if (speed >= 3500000)
        return 3500000;
    else
#endif
...
    if (speed >= 57600)
        return 57600;
    else if (speed >= 38400)
        return 38400;
    else if (speed >= 19200)
        return 19200;
    else if (speed >= 9600)
        return 9600;
    else if (speed >= 4800)
        return 4800;
    else if (speed >= 2400)
        return 2400;
    else
        return 1200;
}


> Maybe we should check what the BSDs or any other implementations do
> here...
>

Jeff

[-- Attachment #2: Type: text/html, Size: 7330 bytes --]