From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5B6BEC001DE for ; Tue, 25 Jul 2023 17:33:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229694AbjGYRdl (ORCPT ); Tue, 25 Jul 2023 13:33:41 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38676 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229631AbjGYRdl (ORCPT ); Tue, 25 Jul 2023 13:33:41 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 584CAA6; Tue, 25 Jul 2023 10:33:40 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id E1EE561839; Tue, 25 Jul 2023 17:33:39 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1BFF8C433C8; Tue, 25 Jul 2023 17:33:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1690306419; bh=W2bYqQuaVMZ/wZjaKxCd2Tdu6cVEZXTe5IXAu3PvR+w=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=yg9Es6XWE4Bo/8JKnt60txUTkQl0JRQUI4uswwHVwEB3upOVVO4xDsRJKLDT6gBg0 3tAuVPbKVZ0+gahmOyTEgoLM53esIdlo5LyHYjylgx6AT9Xg+kcCEwS2Wg8BM9L7LZ NtrseQYGXpgVUsiwT9EAZ/zj2DO8pLmo4n/EK/P0= Date: Tue, 25 Jul 2023 19:33:36 +0200 From: Greg Kroah-Hartman To: Chunyan Zhang Cc: Chunyan Zhang , Jiri Slaby , linux-serial@vger.kernel.org, Baolin Wang , Orson Zhai , LKML Subject: Re: [PATCH V3 1/2] serial: sprd: Assign sprd_port after initialized to avoid wrong access Message-ID: <2023072551-surrogate-reproach-3634@gregkh> References: <20230725064053.235448-1-chunyan.zhang@unisoc.com> <2023072548-jolliness-unbolted-621c@gregkh> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-serial@vger.kernel.org On Tue, Jul 25, 2023 at 03:49:15PM +0800, Chunyan Zhang wrote: > On Tue, 25 Jul 2023 at 14:50, Greg Kroah-Hartman > wrote: > > > > On Tue, Jul 25, 2023 at 02:40:52PM +0800, Chunyan Zhang wrote: > > > The global pointer 'sprd_port' may not zero when sprd_probe returns > > > failure, that is a risk for sprd_port to be accessed afterward, and > > > may lead to unexpected errors. > > > > > > For example: > > > > > > There are two UART ports, UART1 is used for console and configured in > > > kernel command line, i.e. "console="; > > > > > > The UART1 probe failed and the memory allocated to sprd_port[1] was > > > released, but sprd_port[1] was not set to NULL; > > > > > > In UART2 probe, the same virtual address was allocated to sprd_port[2], > > > and UART2 probe process finally will go into sprd_console_setup() to > > > register UART1 as console since it is configured as preferred console > > > (filled to console_cmdline[]), but the console parameters (sprd_port[1]) > > > belong to UART2. > > > > > > So move the sprd_port[] assignment to where the port already initialized > > > can avoid the above issue. > > > > > > Fixes: b7396a38fb28 ("tty/serial: Add Spreadtrum sc9836-uart driver support") > > > Signed-off-by: Chunyan Zhang > > > --- > > > V3: > > > - Call uart_unregister_driver() only when the 'sprd_ports_num' decreases to 0; > > > - Add calling sprd_rx_free_buf() instread of sprd_remove() under clean_up lable. > > > > > > V2: > > > - Leave sprd_remove() to keep the unrelated code logic the same. > > > --- > > > drivers/tty/serial/sprd_serial.c | 25 +++++++++++++++++-------- > > > 1 file changed, 17 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/tty/serial/sprd_serial.c b/drivers/tty/serial/sprd_serial.c > > > index b58f51296ace..fc1377029021 100644 > > > --- a/drivers/tty/serial/sprd_serial.c > > > +++ b/drivers/tty/serial/sprd_serial.c > > > @@ -1106,7 +1106,7 @@ static bool sprd_uart_is_console(struct uart_port *uport) > > > static int sprd_clk_init(struct uart_port *uport) > > > { > > > struct clk *clk_uart, *clk_parent; > > > - struct sprd_uart_port *u = sprd_port[uport->line]; > > > + struct sprd_uart_port *u = container_of(uport, struct sprd_uart_port, port); > > > > Now that you are not allocaing the sprd_port[] pointers, shouldn't you > > also remove that variable entirely? > > sprd_console_write() and sprd_console_setup() [1] also need sprd_port[]. Why? Can't they also use the structure passed to them instead? > So, this driver still needs to allocate the buffer for sprd_port[], > the change is using a local variable instead of allocating directly to > the global pointer. Ah, I missed that you were saving the pointer off. I think it would be better if you could just remove the static array entirely, that's a sign of a very old driver that should be fixed up. thanks, greg k-h