Message ID | 20231014104942.856152-2-vamshigajjela@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:2908:b0:403:3b70:6f57 with SMTP id ib8csp2411172vqb; Sat, 14 Oct 2023 03:50:26 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEVj+hkQy4PfbZUk09tyldWLpiZhxsmGERRBJZ8GNiixpcJDIFjaCk36Z7cOiNglSeLIUbL X-Received: by 2002:a17:902:ce86:b0:1c4:2b71:7dc9 with SMTP id f6-20020a170902ce8600b001c42b717dc9mr32846373plg.4.1697280626375; Sat, 14 Oct 2023 03:50:26 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1697280626; cv=none; d=google.com; s=arc-20160816; b=OCWarsN3VSQqZZcYmaFW2yJ8t5iIskgqGD/EcBVSlIqjfiSRf932iY7t/yYBy4wtgX /WQbPe2B/KencVZvnTmt11hUxd7FscO1DbOa1A9r05suibfroyUdjj2wTrsWeUa7k4Vt GyiG0MRZPHzP9prtXGnBbH1B40Cw6m51UgPOnZ1KAG7SkvZc8H7e0zIJasgF22hzoPwD TeH2SrWB+EuX+6yWiqX6PHdavepog0CYD6kLGuroGYCppeQFyRGfdsJWNMz16kybQHQq ZCKtrq+6/TP9iCRncSvQhPoRKanSAHSRNeOKjPYSgoOKfCpc2blS+nOrhnHoCgk2s9zv dtfA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:from:subject:message-id:references :mime-version:in-reply-to:date:dkim-signature; bh=rhmpXagUV1Eroi8NsqxMkQ7rrh98dWj1q7PH6sEyJW0=; fh=4iPUH3SCaUeHRPwZInlqPLZI8cmh/b74OgcORLvvuVM=; b=a19panFrekT5vBllRaEdOQmKzRUbtPknfq1eS+1OppxEVJ2Abp3JcPDGxxBKbvXJsB AwzyI8XRdp6fKcqLqOLDtlHLFMWuWskyc0Ys8S3u+4sixw27yJA9e1Ek7IwRGe2DRw6J gXbcVkHymVAN4ibyrlthDEFrmj166zNiLpAev1g//EDnp/9UeOdO5kVKGU3rrWlPcPaU jvTo46O2VaQJ50R6VB8ay1xhjZhdfs+qyT7+J0eLlqrkQZPrrxhEaGGRSJr6DgV3V/Dz M1cG+aegEvfzBd1m7FW2H1iOKqC6KkudCaaImXUKrYBNgUmRr0Fxc8sVNC0pWX9xcGLc Y0UQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=wcNgefQ4; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from snail.vger.email (snail.vger.email. [2620:137:e000::3:7]) by mx.google.com with ESMTPS id c23-20020a170902b69700b001b674055d72si6179048pls.621.2023.10.14.03.50.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 14 Oct 2023 03:50:26 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=wcNgefQ4; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by snail.vger.email (Postfix) with ESMTP id 696D3802DA93; Sat, 14 Oct 2023 03:50:25 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at snail.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233126AbjJNKuF (ORCPT <rfc822;hjfbswb@gmail.com> + 20 others); Sat, 14 Oct 2023 06:50:05 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46128 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233104AbjJNKuD (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sat, 14 Oct 2023 06:50:03 -0400 Received: from mail-yw1-x1149.google.com (mail-yw1-x1149.google.com [IPv6:2607:f8b0:4864:20::1149]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C273AA2 for <linux-kernel@vger.kernel.org>; Sat, 14 Oct 2023 03:50:01 -0700 (PDT) Received: by mail-yw1-x1149.google.com with SMTP id 00721157ae682-59b5a586da6so22474137b3.1 for <linux-kernel@vger.kernel.org>; Sat, 14 Oct 2023 03:50:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1697280601; x=1697885401; darn=vger.kernel.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=rhmpXagUV1Eroi8NsqxMkQ7rrh98dWj1q7PH6sEyJW0=; b=wcNgefQ4O/Qa+X1vZ1a6Prnxsn9zQBeunBJaQVtybAUSD8Ps2zSAkmGhgoLYdg1Znm MBHVq+P9Hfg0Ib/V59SJqAPgIGvaDLmsx70bCwly+H9Ackta0hUpZh/u4lY3tqQT1enm 7v23y44PpSgJ6aKqzw1kDF4fJZ63fEkKeIy8YePGLs4ynhRQU80+NLd0qPdQAZ840esq WzPgKxEMgxNdPlW6FW9apro65szce1bKLo3cPshn5D4rJSkHRiAFFHrshRTmWwRn3hsT f9PHGjyVLLywrlKJTJJupeiNl3peIEdWhzUGz18819QmQMP6Le8w8R9ZL0znau7WGThl AJpA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697280601; x=1697885401; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=rhmpXagUV1Eroi8NsqxMkQ7rrh98dWj1q7PH6sEyJW0=; b=Voo9oPiuazxX4aQmojKiYY1j4mRGfKcgIX9PA//P5oXtuxaWm4RVh6AEchSBHC5fni 8Ad41tS2bZ2t/VXWmhQIjU/D4J0HPffYw+AfuLHjReTU7iMkXRVkBmgdnx4lCzfQpCQW 1Gj9MQakhPxLRBBgdVkNv3wtlP8oW/bGJbmJosLBDc2EiWEoGN8yF5mjwweeJ+B7Ad2u FskOIA4qgOC/q0fURemb8XXW/GBvymmBF5/cokO3MvFNdpu40J3Ikv0Vau5FV0rsI7tC TaYrr54JxDs2vSZiTi0aQX24uCz5lWvgu5OLCZyHywDqE0eDb0UQUYo4wZX3GmtgcIrs C2WA== X-Gm-Message-State: AOJu0YyRTQ2RDSQCfDC3KWYpr5mKCfkGOYw9v5wScmEdXyhbP5mLLQzx PxtfclwKvnaqZJR0DVmZLcJb+UZURQLWJuNjnvxl X-Received: from vamshig51.c.googlers.com ([fda3:e722:ac3:cc00:3:22c1:c0a8:70c]) (user=vamshigajjela job=sendgmr) by 2002:a05:690c:368b:b0:5a7:a731:3360 with SMTP id fu11-20020a05690c368b00b005a7a7313360mr88331ywb.2.1697280601060; Sat, 14 Oct 2023 03:50:01 -0700 (PDT) Date: Sat, 14 Oct 2023 16:19:40 +0530 In-Reply-To: <20231014104942.856152-1-vamshigajjela@google.com> Mime-Version: 1.0 References: <20231014104942.856152-1-vamshigajjela@google.com> X-Mailer: git-send-email 2.42.0.655.g421f12c284-goog Message-ID: <20231014104942.856152-2-vamshigajjela@google.com> Subject: [PATCH 1/3] serial: core: Potential overflow of frame_time From: Vamshi Gajjela <vamshigajjela@google.com> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Jiri Slaby <jirislaby@kernel.org>, ilpo.jarvinen@linux.intel.com Cc: linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org, manugautam@google.com, Subhash Jadavani <sjadavani@google.com>, Channa Kadabi <kadabi@google.com>, VAMSHI GAJJELA <vamshigajjela@google.com> Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF, RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Sat, 14 Oct 2023 03:50:25 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1779727730342289364 X-GMAIL-MSGID: 1779727730342289364 |
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
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>
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 --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;