diff mbox series

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

Message ID 20231014104942.856152-2-vamshigajjela@google.com
State New
Headers
Series serial core type consistency and overflow checks |

Commit Message

Vamshi Gajjela Oct. 14, 2023, 10:49 a.m. UTC 
  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>
---
 include/linux/serial_core.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Ilpo Järvinen Oct. 16, 2023, 10:33 a.m. UTC | #1 
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 but 
the result is always smaller than what requires u64. If you insist on 
doing something add the cast there.

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

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

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 all 
the other cases where 64-bit arithmetic needs to be used in the kernel be 
similarly upconverted to 64 bits?

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

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ärvinen <ilpo.jarvinen@linux.intel.com>
Vamshi Gajjela Oct. 18, 2023, 1:45 p.m. UTC | #2 
On Mon, Oct 16, 2023 at 4:03 PM Ilpo Järvinen
<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 but
> 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 = 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 patch.
>
> 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 all
> 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 = 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ärvinen <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 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;
> >
diff mbox series

Patch

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;