Re: [PATCH v2 1/3] serial: core: Potential overflow of frame_time

[VAMSHI GAJJELA]

On Sun, Oct 15, 2023 at 4:44 AM Hugo Villeneuve <hugo@xxxxxxxxxxx> wrote:
>
> On Sat, 14 Oct 2023 23:43:33 +0530
> Vamshi Gajjela <vamshigajjela@xxxxxxxxxx> wrote:
>
> > From: VAMSHI GAJJELA <vamshigajjela@xxxxxxxxxx>
>
> Hi,
> your commit title doesn't really explain what this patch is doing.
>
> Please see: https://cbea.ms/git-commit/#imperative
Thanks Hugo for the review, I will provide details in the following response.
>
>
> > uart_update_timeout() sets a u64 value to an unsigned int frame_time.
> > While it can be cast to u32 before assignment, there's a specific case
> > where frame_time is cast to u64. Since frame_time consistently
> > participates in u64 arithmetic, its data type is converted to u64 to
> > eliminate the need for explicit casting.
>
> Again, refering to your title commit message, I would expect that you
> would describe precisely how a potential overflow can happen here, and
> I am not seeing it.
>
> And based on your log message, it seems that your commit is simply some
> kind of optimization, not a fix?

In the function uart_update_timeout() within serial_core.c, a u64 value is
assigned to an "unsigned int" variable frame_time. This raises concerns about
potential overflow. While the code in the patch doesn't explicitly manifest
the issue in the following line of uart_update_timeout()

"port->frame_time = DIV64_U64_ROUND_UP(frame_time, baud);"

lacks a u32 typecast for frame_time.

In the function "uart_fifo_timeout" has the following line of code

u64 fifo_timeout = (u64)READ_ONCE(port->frame_time) * port->fifosize;

In the above line frame_time is typecast to u64

also, timeout values in the serial core associated with frame_time are used
in the u64 arithmetic. To maintain consistency and readability, I've updated
the size of frame_time from "unsigned int" to "unsigned long". This ensures
uniformity with typecasts elsewhere in the code, addressing potential issues
and enhancing clarity.

I hope this provides clarity. Would you find it helpful if I were to provide
further details in the commit message?
>
> Hugo.
>
>
>
> > Signed-off-by: VAMSHI GAJJELA <vamshigajjela@xxxxxxxxxx>
> > ---
> > v2:
> > - use DIV64_U64_ROUND_UP with frame_time
> >
> >  drivers/tty/serial/8250/8250_port.c | 2 +-
> >  include/linux/serial_core.h         | 4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> > index 141627370aab..d1bf794498c4 100644
> > --- a/drivers/tty/serial/8250/8250_port.c
> > +++ b/drivers/tty/serial/8250/8250_port.c
> > @@ -1510,7 +1510,7 @@ static inline void __stop_tx(struct uart_8250_port *p)
> >                        * rather than after it is fully sent.
> >                        * Roughly estimate 1 extra bit here with / 7.
> >                        */
> > -                     stop_delay = p->port.frame_time + DIV_ROUND_UP(p->port.frame_time, 7);
> > +                     stop_delay = p->port.frame_time + DIV64_U64_ROUND_UP(p->port.frame_time, 7);
> >               }
> >
> >               __stop_tx_rs485(p, stop_delay);
> > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> > index bb6f073bc159..b128513b009a 100644
> > --- a/include/linux/serial_core.h
> > +++ b/include/linux/serial_core.h
> > @@ -558,7 +558,7 @@ struct uart_port {
> >
> >       bool                    hw_stopped;             /* sw-assisted CTS flow state */
> >       unsigned int            mctrl;                  /* current modem ctrl settings */
> > -     unsigned int            frame_time;             /* frame timing in ns */
> > +     unsigned long           frame_time;             /* frame timing in ns */
> >       unsigned int            type;                   /* port type */
> >       const struct uart_ops   *ops;
> >       unsigned int            custom_divisor;
> > @@ -764,7 +764,7 @@ unsigned int uart_get_divisor(struct uart_port *port, unsigned int baud);
> >   */
> >  static inline unsigned long uart_fifo_timeout(struct uart_port *port)
> >  {
> > -     u64 fifo_timeout = (u64)READ_ONCE(port->frame_time) * port->fifosize;
> > +     u64 fifo_timeout = READ_ONCE(port->frame_time) * port->fifosize;
> >
> >       /* Add .02 seconds of slop */
> >       fifo_timeout += 20 * NSEC_PER_MSEC;
> > --
> > 2.42.0.655.g421f12c284-goog
> >