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 91A17CCA481 for ; Mon, 27 Jun 2022 14:04:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236628AbiF0OEi (ORCPT ); Mon, 27 Jun 2022 10:04:38 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42356 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236581AbiF0OEf (ORCPT ); Mon, 27 Jun 2022 10:04:35 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [IPv6:2604:1380:4601:e00::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1B63C11C2E for ; Mon, 27 Jun 2022 07:04:35 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id BBDAFB81731 for ; Mon, 27 Jun 2022 14:04:33 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1DB38C3411D; Mon, 27 Jun 2022 14:04:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1656338672; bh=zEszrmJlOAeJrRQwz8LemoYGJZg8EtQPIqnT2jiXKZo=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ZiXnsPyv3Ka49CWGlAQ0xGFf0fUR84r+A3zXvFbHGt6pXG6FSaLrVR5D1jrj1HnS7 4o4H8b72kMyJT+6QJMGRBcC3uc1qOwBg6/IOnLsgvXg5QcKO77wC3uqMLCsaLe9mpj MCMW3LwlQPSeziZGraOF6ekp40aygF/+grHayqoQ= Date: Mon, 27 Jun 2022 16:04:29 +0200 From: Greg KH To: Shreenidhi Shedi Cc: arnd@arndb.de, linux-kernel@vger.kernel.org, Shreenidhi Shedi Subject: Re: [PATCH 1/2] char: lp: ensure that index has not exceeded LP_NO Message-ID: References: <20220603130040.601673-1-sshedi@vmware.com> <554b5c77-12ba-7b85-a65a-f1fefa5b6a51@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <554b5c77-12ba-7b85-a65a-f1fefa5b6a51@gmail.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Jun 11, 2022 at 11:15:26AM +0530, Shreenidhi Shedi wrote: > On 10/06/22 8:00 pm, Greg KH wrote: > > On Fri, Jun 10, 2022 at 07:12:02PM +0530, Shreenidhi Shedi wrote: > >> On 10/06/22 6:58 pm, Greg KH wrote: > >>> On Fri, Jun 03, 2022 at 06:30:39PM +0530, Shreenidhi Shedi wrote: > >>>> From: Shreenidhi Shedi > >>>> > >>>> After finishing the loop, index value can be equal to LP_NO and lp_table > >>>> array is of size LP_NO, so this can end up in accessing an out of bound > >>>> address in lp_register function. > >>>> > >>>> Signed-off-by: Shreenidhi Shedi > >>>> --- > >>>> drivers/char/lp.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/char/lp.c b/drivers/char/lp.c > >>>> index 0e22e3b0a..d474d02b6 100644 > >>>> --- a/drivers/char/lp.c > >>>> +++ b/drivers/char/lp.c > >>>> @@ -972,7 +972,7 @@ static void lp_attach(struct parport *port) > >>>> if (port_num[i] == -1) > >>>> break; > >>>> > >>>> - if (!lp_register(i, port)) > >>>> + if (i < LP_NO && !lp_register(i, port)) > >>>> lp_count++; > >>> > >>> How can this ever be needed? Look at the check further up for the check > >>> of lp_count which prevents this from every going too large. > >>> > >>> So how can an address be accessed out of bound here? > >>> > >>> thanks, > >>> > >>> greg k-h > >> > >> Thanks for the review. Assume lp_count is less than LP_NO now and we enter the for loop > >> and for some reason for loop exits after i reaching the value LP_NO > > > > Wait, how can that happen? That's what I am saying, the loop will never > > reach that value from what I can tell. > > > > Yes, this whole thing should be moved to something more sane like an > > idr structure, but as-is, it seems correct to me. > > > > Have you tested the code with that many devices to see if it really can > > overflow? > > > > thanks, > > > > greg k-h > > No, I did not actually test it with real hardware but I ran a > static analyzer check on this file and it also thinks the same. Please try it on real hardware. static tools are not always the smartest thing out there. good luck! greg k-h