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 7F87EC433F5 for ; Wed, 16 Feb 2022 15:11:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235404AbiBPPMA (ORCPT ); Wed, 16 Feb 2022 10:12:00 -0500 Received: from mxb-00190b01.gslb.pphosted.com ([23.128.96.19]:52372 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233655AbiBPPL7 (ORCPT ); Wed, 16 Feb 2022 10:11:59 -0500 X-Greylist: delayed 477 seconds by postgrey-1.37 at lindbergh.monkeyblade.net; Wed, 16 Feb 2022 07:11:46 PST Received: from smtp97.iad3b.emailsrvr.com (smtp97.iad3b.emailsrvr.com [146.20.161.97]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5268A2AE07; Wed, 16 Feb 2022 07:11:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=mev.co.uk; s=20190130-41we5z8j; t=1645023828; bh=F0I1g8Iodxs1TYYZfhyaZMhWxcIOGrZzwTJS2frSzlM=; h=Date:Subject:To:From:From; b=h++3Om8rBhbI8ndeetZ+++N3SJPManU2ui6H2e+ipO6IqZyH5SsxrEtxJnU3sD+/M cKDVCemBkY5ikgP3+My0iLOXlGWxYiXgRwqndS/6A1InO65cPVXaYg0L3mGHm+137J XR+/3p8aU9/knd8xVMad9t8ceGIR8c6jgdIVyNhI= X-Auth-ID: abbotti@mev.co.uk Received: by smtp5.relay.iad3b.emailsrvr.com (Authenticated sender: abbotti-AT-mev.co.uk) with ESMTPSA id B4D24400A5; Wed, 16 Feb 2022 10:03:46 -0500 (EST) Message-ID: Date: Wed, 16 Feb 2022 15:03:45 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.1 Subject: Re: [PATCH] comedi: drivers: ni_routes: Use strcmp() instead of memcmp() Content-Language: en-GB To: Kees Cook Cc: H Hartley Sweeten , "Spencer E . Olson" , Greg Kroah-Hartman , Masahiro Yamada , Lee Jones , kernel test robot , Nathan Chancellor , Nick Desaulniers , linux-kernel@vger.kernel.org, llvm@lists.linux.dev, linux-hardening@vger.kernel.org References: <20220215171017.1247291-1-keescook@chromium.org> From: Ian Abbott Organization: MEV Ltd. In-Reply-To: <20220215171017.1247291-1-keescook@chromium.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Classification-ID: c69dd1e8-1590-4aa0-802d-b75f2af30825-1-1 Precedence: bulk List-ID: X-Mailing-List: linux-hardening@vger.kernel.org On 15/02/2022 17:10, Kees Cook wrote: > The family and device comparisons were using memcmp(), but this could > lead to Out-of-bounds reads when the length was larger than the > buffers being compared. Since these appear to always be NUL-terminated > strings, just use strcmp() instead. > > This was found with Clang under LTO: > > [ 92.405851][ T1] kernel BUG at lib/string_helpers.c:980! > ... > [ 92.409141][ T1] RIP: 0010:fortify_panic (fbdev.c:?) > ... > [ 92.410056][ T1] ni_assign_device_routes (fbdev.c:?) > [ 92.410056][ T1] ? unittest_enter (fbdev.c:?) > [ 92.410056][ T1] ni_routes_unittest (ni_routes_test.c:?) > [ 92.410056][ T1] ? unittest_enter (fbdev.c:?) > [ 92.410056][ T1] __initstub__kmod_ni_routes_test__505_604_ni_routes_unittest6 (fbdev.c:?) > [ 92.410056][ T1] do_one_initcall (fbdev.c:?) > > Cc: Ian Abbott > Cc: H Hartley Sweeten > Cc: Spencer E. Olson > Cc: Greg Kroah-Hartman > Cc: Masahiro Yamada > Cc: Lee Jones > Reported-by: kernel test robot > Link: https://lore.kernel.org/lkml/20220210072821.GD4074@xsang-OptiPlex-9020 > Fixes: 4bb90c87abbe ("staging: comedi: add interface to ni routing table information") > Signed-off-by: Kees Cook > --- > drivers/comedi/drivers/ni_routes.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/comedi/drivers/ni_routes.c b/drivers/comedi/drivers/ni_routes.c > index f24eeb464eba..295a3a9ee0c9 100644 > --- a/drivers/comedi/drivers/ni_routes.c > +++ b/drivers/comedi/drivers/ni_routes.c > @@ -56,8 +56,7 @@ static const u8 *ni_find_route_values(const char *device_family) > int i; > > for (i = 0; ni_all_route_values[i]; ++i) { > - if (memcmp(ni_all_route_values[i]->family, device_family, > - strnlen(device_family, 30)) == 0) { > + if (!strcmp(ni_all_route_values[i]->family, device_family)) { > rv = &ni_all_route_values[i]->register_values[0][0]; > break; > } > @@ -75,8 +74,7 @@ ni_find_valid_routes(const char *board_name) > int i; > > for (i = 0; ni_device_routes_list[i]; ++i) { > - if (memcmp(ni_device_routes_list[i]->device, board_name, > - strnlen(board_name, 30)) == 0) { > + if (!strcmp(ni_device_routes_list[i]->device, board_name)) { > dr = ni_device_routes_list[i]; > break; > } Looks good, thanks! I'm not sure why the tests used memcmp() like that. Indeed, all the strings are statically allocated and null-terminated. Reviewed-by: Ian Abbott -- -=( Ian Abbott || MEV Ltd. is a company )=- -=( registered in England & Wales. Regd. number: 02862268. )=- -=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=- -=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-