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 6FFB4C433EF for ; Tue, 18 Jan 2022 02:18:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S243970AbiARCSX (ORCPT ); Mon, 17 Jan 2022 21:18:23 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56014 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238976AbiARCSW (ORCPT ); Mon, 17 Jan 2022 21:18:22 -0500 Received: from mail-pg1-x544.google.com (mail-pg1-x544.google.com [IPv6:2607:f8b0:4864:20::544]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 988D9C06161C for ; Mon, 17 Jan 2022 18:18:22 -0800 (PST) Received: by mail-pg1-x544.google.com with SMTP id i8so12503957pgt.13 for ; Mon, 17 Jan 2022 18:18:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=M5EU8xSyfUERpd7Pq8+3cm8Q6XTji9bSKcDdI+RBBQA=; b=LOXWor/YiEKB8CeYc/LTnk0axctKze7m+mK5JOMZrsUYSFvwaOgfPr7m3n3lxR0mpj /lL6uX3dUilpNbjXQTryKMTwYWHwu/G2/yfiVoPWYDxEyixzuRujNghSqgLY5e9WdSDm HWUz5tB1w8w5JjrlVlTaY3x4aan36aX4X6BwiFiUTfsiXIU0eOh6GY+shXO/Ypdhqzh+ qSvIGhm5im2NGbFc0y0Fjq8GQOkTeveMcI/fcP95Y82gGfvE+zXx296gw+U3hz0SCqeO piWbjy2/O/0Fm02Ai5iwiJVCsegUvCoRiHg75B3t/b2mBr4b//Zi0hTpr5DRxv6Nt2O9 Qx1Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=M5EU8xSyfUERpd7Pq8+3cm8Q6XTji9bSKcDdI+RBBQA=; b=HrG0jtUvLS3eLB4G/Ssfk/Y9KrTmFuN1TOxjmluC9WrbAw0Mvx9LK91KWMzvienX4V PBAiwTd6z0C3Rn0TrvlqN1mYR+eoJzuZPQG2USlsTlA6WY9sq+uR12wVGeYuC2SH8g8y uh0KZyGFvg0zhRoXuSqHFEJX55uYrEYQVbEasqor7ahBWttMEdLcqBakXSCWyfHU/F7s zKzG+YsVn8hZbp/p54ykt/ZEf/T1jm6tQGFT2AkmRc4HdPINp0HV7W2SY+j3gqy/zX1A jVstzX82NcApGcvkVSSzoTCDxeCeeGq0Arb0ZC4zVQcEg+sYUEWk6Smif3VGeKFZGrEE Q44g== X-Gm-Message-State: AOAM531iyASlEVClLRXNR61NTry3IUFu+eQYdgPskvOhybcRvAvvrTsv Z+Cbac/Lccot/Wen5oKpAXk= X-Google-Smtp-Source: ABdhPJzM0s6kDprNQgEL8WGfViL8HILHhl5tTlI2W1Ft8DEEJbNAw/LXMtU8HN7ogIiAgQFEjms/ng== X-Received: by 2002:a62:528f:0:b0:4bc:d18e:cc20 with SMTP id g137-20020a62528f000000b004bcd18ecc20mr23603855pfb.40.1642472301796; Mon, 17 Jan 2022 18:18:21 -0800 (PST) Received: from [0.0.0.0] ([209.97.166.32]) by smtp.gmail.com with ESMTPSA id e3sm13366598pgl.59.2022.01.17.18.18.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 17 Jan 2022 18:18:21 -0800 (PST) Subject: Re: [PATCH v2 RESEND] m68k/kernel: array out of bound access in process_uboot_commandline To: Greg Ungerer , geert@linux-m68k.org Cc: schwab@linux-m68k.org, linux-m68k@lists.linux-m68k.org, linux-kernel@vger.kernel.org References: <20220113015854.9326-1-hbh25y@gmail.com> <95d91f5f-c5e1-0750-ebb9-b6839aecdc7c@linux-m68k.org> From: Hangyu Hua Message-ID: <9775e266-5fee-b0e9-7fa3-b602ec4b7796@gmail.com> Date: Tue, 18 Jan 2022 10:18:16 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <95d91f5f-c5e1-0750-ebb9-b6839aecdc7c@linux-m68k.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-m68k@vger.kernel.org Hi Greg, On 2022/1/17 下午12:03, Greg Ungerer wrote: > Hi Hangyu, > > On 13/1/22 11:58 am, Hangyu Hua wrote: >> When the size of commandp >= size, array out of bound write occurs >> because >> len == 0. >> >> Signed-off-by: Hangyu Hua >> --- >>   arch/m68k/kernel/uboot.c | 3 ++- >>   1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/m68k/kernel/uboot.c b/arch/m68k/kernel/uboot.c >> index 928dbd33fc4a..63eaf3c3ddcd 100644 >> --- a/arch/m68k/kernel/uboot.c >> +++ b/arch/m68k/kernel/uboot.c >> @@ -101,5 +101,6 @@ __init void process_uboot_commandline(char >> *commandp, int size) >>       } >>       parse_uboot_commandline(commandp, len); >> -    commandp[len - 1] = 0; >> +    if (len > 0) >> +        commandp[len - 1] = 0; >>   } >> > > I am not convinced this is wrong for the reason you think it is. > Looking at the code in its entirety: > > __init void process_uboot_commandline(char *commandp, int size) > { >         int len, n; > >         n = strnlen(commandp, size); >         commandp += n; >         len = size - n; >         if (len) { >                 /* Add the whitespace separator */ >                 *commandp++ = ' '; >                 len--; >         } > >         parse_uboot_commandline(commandp, len); >         commandp[len - 1] = 0; > } > > > "commandp" is moved based on the return of the strnlen(). So in the > case of commandp actually being full of valid characters (so n == size, > and thus len == 0) the commandp technically points outside of its > real size at that point. But "command[[len - 1]" would actually be > pointing to the last char in the original commandp array (so the original > commandp[size - 1]). Well at least if you are happy with the use of > negative array indexes. > You mean this is a friendly out of bound beacause "command[[len - 1]" pointing to the last char in the original commandp array. I used to think command[[len - 1] = 0 may be a zero-terminated for command. You can see my discussion with Andreas Schwab and my patch v1 in https://lore.kernel.org/all/CAOo-nLJG71QqqD0-cJDyH0rY2VTx1eO9nHVQ5MCe8J0iiME_vw@mail.gmail.com/ But this still be a out of bound write because "commandp" is a macro definition with a fixed size. > Clearly this could be structured better. There is no point in calling > parse_uboot_commandline() if len == 0, or even if len == 1, since you > cannot add anymore to the command line, it is full. > I think it is no point too. But the caller (setup_arch()) don't check the size of "commandp" before call parse_uboot_commandline(). Instead we do this in parse_uboot_commandline(). So it may be better to move these checks to the caller ? > Regards > Greg Thanks for your reply Hangyu Hua