Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
MIME-Version: 1.0
References: <20231014104942.856152-1-vamshigajjela@google.com>
 <20231014104942.856152-2-vamshigajjela@google.com> <6f25e6fb-bebc-3f9b-9876-5e14d2582f6@linux.intel.com>
In-Reply-To: <6f25e6fb-bebc-3f9b-9876-5e14d2582f6@linux.intel.com>
From:   VAMSHI GAJJELA <vamshigajjela@google.com>
Date:   Wed, 18 Oct 2023 19:15:07 +0530
Message-ID: <CAMTSyjqc118-by6LRHaSN7k8fOcR6K0kmYXdthPD7rqJuYOaVw@mail.gmail.com>
Subject: Re: [PATCH 1/3] serial: core: Potential overflow of frame_time
To:     =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        linux-serial <linux-serial@vger.kernel.org>,
        LKML <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 Mon, Oct 16, 2023 at 4:03=E2=80=AFPM Ilpo J=C3=A4rvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Sat, 14 Oct 2023, Vamshi Gajjela wrote:
>
> > From: VAMSHI GAJJELA <vamshigajjela@google.com>
> >
> > uart_update_timeout() sets a u64 value to an unsigned int frame_time.
>
> Yes it does, because uart_update_timeout() does math that requires u64 bu=
t
> the result is always smaller than what requires u64. If you insist on
> doing something add the cast there.
Agree, will add a cast there. Can I do that as part of the patch series 2/3
+     u64 size =3D tty_get_frame_size(cflag);
in uart_update_timeout
>
> > While it can be cast to u32 before assignment, there's a specific case
> > where frame_time is cast to u64.
>
> Because it gets multipled with something that results in a big number
> The cast is all correct too because the developer actually thought of
> possiblity of an overflow on multiply (something every developer should
> be conscious of), so there's nothing to see there either.
Yes, nothing wrong.
>
> > Since frame_time consistently
> > participates in u64 arithmetic, its data type is converted to u64 to
> > eliminate the need for explicit casting.
>
> You need a way more convincing argument that that since you're not even
> converting it to u64 like you falsely stated so the sizes still won't
> match on all architectures.
"all architectures." is something I have missed while considering this patc=
h.
>
> I see you've realized u32 is more than enough to store frame time for the
> speeds UART operates with? So why exactly is this patch needed? Should al=
l
> the other cases where 64-bit arithmetic needs to be used in the kernel be
> similarly upconverted to 64 bits?
Certainly not for other 64-bit arithmetics, I will course correct.
yes u32 is sufficient to store frame time.
>
> Also, did you happen to realize frame_time also participates in 32-bit
> arithmetic which you just make much worse with this change? (Yes, there
> are 32-bit divides done for it.)
char_time =3D max(nsecs_to_jiffies(port->frame_time / 5), 1UL);
Here is that instance


>
> So NACK from me to this "fix" of a non-problem by causing much worse
> problems you seem to be entirely unaware.
>
> NACKED-by:  Ilpo J=C3=A4rvinen <ilpo.jarvinen@linux.intel.com>
>
> --
>  i.
>
> > Signed-off-by: VAMSHI GAJJELA <vamshigajjela@google.com>
> > ---
> >  include/linux/serial_core.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > 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;
> >