Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
MIME-Version: 1.0
References: <20231014181333.2579530-1-vamshigajjela@google.com> <20231014191355.7f44aac3537a8d260225fa0c@hugovil.com>
In-Reply-To: <20231014191355.7f44aac3537a8d260225fa0c@hugovil.com>
From:   VAMSHI GAJJELA <vamshigajjela@google.com>
Date:   Sun, 15 Oct 2023 23:19:47 +0530
Message-ID: <CAMTSyjohXMQVqesqWsqd=vfRGEhqFB=MBnh8ZRk3hjHOXLdEkg@mail.gmail.com>
Subject: Re: [PATCH v2 1/3] serial: core: Potential overflow of frame_time
To:     Hugo Villeneuve <hugo@hugovil.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        ilpo.jarvinen@linux.intel.com, linux-serial@vger.kernel.org,
        linux-kernel@vger.kernel.org, manugautam@google.com,
        Subhash Jadavani <sjadavani@google.com>,
        Channa Kadabi <kadabi@google.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, Oct 15, 2023 at 4:44=E2=80=AFAM Hugo Villeneuve <hugo@hugovil.com> =
wrote:
>
> On Sat, 14 Oct 2023 23:43:33 +0530
> Vamshi Gajjela <vamshigajjela@google.com> wrote:
>
> > From: VAMSHI GAJJELA <vamshigajjela@google.com>
>
> 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 respons=
e.
>
>
> > 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 abo=
ut
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 =3D 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 =3D (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 update=
d
the size of frame_time from "unsigned int" to "unsigned long". This ensures
uniformity with typecasts elsewhere in the code, addressing potential issue=
s
and enhancing clarity.

I hope this provides clarity. Would you find it helpful if I were to provid=
e
further details in the commit message?
>
> Hugo.
>
>
>
> > Signed-off-by: VAMSHI GAJJELA <vamshigajjela@google.com>
> > ---
> > 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/8=
250/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_por=
t *p)
> >                        * rather than after it is fully sent.
> >                        * Roughly estimate 1 extra bit here with / 7.
> >                        */
> > -                     stop_delay =3D p->port.frame_time + DIV_ROUND_UP(=
p->port.frame_time, 7);
> > +                     stop_delay =3D p->port.frame_time + DIV64_U64_ROU=
ND_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 CT=
S flow state */
> >       unsigned int            mctrl;                  /* current modem =
ctrl settings */
> > -     unsigned int            frame_time;             /* frame timing i=
n ns */
> > +     unsigned long           frame_time;             /* frame timing i=
n 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 *por=
t, unsigned int baud);
> >   */
> >  static inline unsigned long uart_fifo_timeout(struct uart_port *port)
> >  {
> > -     u64 fifo_timeout =3D (u64)READ_ONCE(port->frame_time) * port->fif=
osize;
> > +     u64 fifo_timeout =3D READ_ONCE(port->frame_time) * port->fifosize=
;
> >
> >       /* Add .02 seconds of slop */
> >       fifo_timeout +=3D 20 * NSEC_PER_MSEC;
> > --
> > 2.42.0.655.g421f12c284-goog
> >