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

  From: VAMSHI GAJJELA <vamshigajjela@google.com>

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.

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(-)
  

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


> 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?

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/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
>
  

On Sun, Oct 15, 2023 at 4:44 AM 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 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@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/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
> >
  

On 14. 10. 23, 20:13, Vamshi Gajjela wrote:
> From: VAMSHI GAJJELA <vamshigajjela@google.com>
> 
> 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.

And make the divisions by the order of magnutude slower for no good 
reason? (Hint: have you looked what DIV64_U64_ROUND_UP() looks like on 
32bit yet?)

Unless you provide a reason it can overflow in real (in fact you spell 
the opposite in the text above):
NACKED-by: Jiri Slaby <jirislaby@kernel.org>

> 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/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;
  

On Mon, 16 Oct 2023, Jiri Slaby wrote:

> On 14. 10. 23, 20:13, Vamshi Gajjela wrote:
> > From: VAMSHI GAJJELA <vamshigajjela@google.com>
> > 
> > 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.
> 
> And make the divisions by the order of magnutude slower for no good reason?
> (Hint: have you looked what DIV64_U64_ROUND_UP() looks like on 32bit yet?)
> 
> Unless you provide a reason it can overflow in real (in fact you spell the
> opposite in the text above):
> NACKED-by: Jiri Slaby <jirislaby@kernel.org>

I sorry but I have to concur Jiri heavily here,

NACKED-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

> > Signed-off-by: VAMSHI GAJJELA <vamshigajjela@google.com>
> > ---
> > v2:
> > - use DIV64_U64_ROUND_UP with frame_time

Please, I barely managed to read your v1 and it's v2 already, don't send 
the next version this soon. There's absolutely no need to fill our inboxes 
with n versions of your patch over a weekend, just remain patient 
and wait reasonable amount of time to gather feedback, please. ...I know 
it's often hard to wait, it's hard for me too at times.

You even failed to convert the other divide done on ->frame_time to 
DIV64_u64_ROUND_UP(), which looks a mindboggling oversight to me.
So far you've managed to cause so many problems with these two attempts to 
fix a non-problem I heavily suggest you focus on something that doesn't 
relate to fixing types. It takes time to understand the related code when 
doing something as simple as type change, which you clearly did not have 
as demonstrated by you missing that other divide which can be trivially 
found with git grep.

> > 
> >   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
> > */

As with v1, u64 != unsigned long, I hope you do know that much about 
different architectures...
  

On Mon, Oct 16, 2023 at 4:30 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Mon, 16 Oct 2023, Jiri Slaby wrote:
>
> > On 14. 10. 23, 20:13, Vamshi Gajjela wrote:
> > > From: VAMSHI GAJJELA <vamshigajjela@google.com>
> > >
> > > 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.
> >
> > And make the divisions by the order of magnutude slower for no good reason?
> > (Hint: have you looked what DIV64_U64_ROUND_UP() looks like on 32bit yet?)
> >
> > Unless you provide a reason it can overflow in real (in fact you spell the
> > opposite in the text above):
> > NACKED-by: Jiri Slaby <jirislaby@kernel.org>
>
> I sorry but I have to concur Jiri heavily here,
>
> NACKED-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>
> > > Signed-off-by: VAMSHI GAJJELA <vamshigajjela@google.com>
> > > ---
> > > v2:
> > > - use DIV64_U64_ROUND_UP with frame_time
>
> Please, I barely managed to read your v1 and it's v2 already, don't send
> the next version this soon. There's absolutely no need to fill our inboxes
> with n versions of your patch over a weekend, just remain patient
> and wait reasonable amount of time to gather feedback, please. ...I know
> it's often hard to wait, it's hard for me too at times.
>
> You even failed to convert the other divide done on ->frame_time to
> DIV64_u64_ROUND_UP(), which looks a mindboggling oversight to me.
> So far you've managed to cause so many problems with these two attempts to
> fix a non-problem I heavily suggest you focus on something that doesn't
> relate to fixing types. It takes time to understand the related code when
> doing something as simple as type change, which you clearly did not have
> as demonstrated by you missing that other divide which can be trivially
> found with git grep.
Apologies for the inconvenience caused, I should have waited for the response
on v1 before making v2, by leaving a comment about the anticipated change.

I have clearly not considered all the architectures in the patch, and
the overhead
of division on 32-archs, mistake from my side. once again apologies for that.

Thanks Jiri Slaby & Ilpo Järvinen for the review.
>
> > >
> > >   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
> > > */
>
> As with v1, u64 != unsigned long, I hope you do know that much about
> different architectures...
>
> --
>  i.
>
> > >     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;
> >
> >