Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] arch:powerpc simple_write_to_buffer return check
To:     Oliver O'Halloran <oohall@gmail.com>
Cc:     Russell Currey <ruscur@russell.cc>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Paul Mackerras <paulus@samba.org>,
        linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <PS1PR04MB29345AB59076B370A4F99F75D6B39@PS1PR04MB2934.apcprd04.prod.outlook.com>
 <CAOSf1CGJ6ZeowMP8Zjo3TazYyaEGuEab4-QRKRJ2jjixUGGtCA@mail.gmail.com>
From:   Mayank Suman <mayanksuman@live.com>
Message-ID: <PS1PR04MB29349F268D8D60A8A43B4507D6B29@PS1PR04MB2934.apcprd04.prod.outlook.com>
Date:   Fri, 5 Feb 2021 11:43:12 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.1
In-Reply-To: <CAOSf1CGJ6ZeowMP8Zjo3TazYyaEGuEab4-QRKRJ2jjixUGGtCA@mail.gmail.com>
Content-Type: text/plain; charset=windows-1252
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from [192.168.0.120] (103.127.101.171) by BMXPR01CA0076.INDPRD01.PROD.OUTLOOK.COM (2603:1096:b00:54::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3825.17 via Frontend Transport; Fri, 5 Feb 2021 06:13:17 +0000
X-MS-PublicTrafficType: Email
X-IncomingHeaderCount: 48
X-EOPAttributedMessage: 0
X-MS-Office365-Filtering-Correlation-Id: 682d8e1f-0987-44bd-504c-08d8c99d2205
X-MS-TrafficTypeDiagnostic: HK2APC01HT207:
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: 0o+kwxJ9AYYchqNZQY8urdb7FQ/KxKRIq6uf1vMh2pspFvmXHhFxOGGrwSs/looSTEMC42U66wh1ENEq2bQ9wiqg02nVS1QmXHn4s/3PHUw1Xvs+4a41pAbOfkShW1Rsaps5yLa1JMYM/5u6tFyzxpxACzQSDfQdKfMLGF3cr6K0Zrjmtw1Y6qGe64jm04ALxhHTDdjMF3Yt3ZvPUzwDB4BgX/k5BT8fweGvBKzRy0S0gCx79NhkKLGzAK+sHmjyIBv6522jD70E0cBWgyhisUGQ5Oo+ft8zfrZCjcPYwKS8pmz74n6yllylDmoMhn5c0XJnftQbCrH3aJ516WcNrx0C9xC0LqEofmd1zamAH7TgwseXu+CCoo2lb/X4mWeRmiyKe/FBFcmPEx8Uw2KzUw==
X-MS-Exchange-AntiSpam-MessageData: WU/18ZY5xlAyi12WhSe2zhUaQDNlLRO3CI8Pc+9B4J3DacMbYKvypMdtDUupX2W+GS6Wi6NsGv+BEf2zEWtQrdV+r1eKNEkQVaEe6+ok7SPkPeB0b1C6Gp+aVdzP9AcHk9o02v/wg8AVhIOworvsJg==
X-OriginatorOrg: live.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 682d8e1f-0987-44bd-504c-08d8c99d2205
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Feb 2021 06:13:19.5638
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-CrossTenant-AuthSource: HK2APC01FT060.eop-APC01.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: Internet
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000
X-MS-Exchange-Transport-CrossTenantHeadersStamped: HK2APC01HT207
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 05/02/21 4:05 am, Oliver O'Halloran wrote:
> On Fri, Feb 5, 2021 at 5:17 AM Mayank Suman <mayanksuman@live.com> wrote:
>>
>> Signed-off-by: Mayank Suman <mayanksuman@live.com>
> 
> commit messages aren't optional

Sorry. I will include the commit message in PATCH v2.

> 
>> ---
>>  arch/powerpc/kernel/eeh.c                    | 8 ++++----
>>  arch/powerpc/platforms/powernv/eeh-powernv.c | 4 ++--
>>  2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
>> index 813713c9120c..2dbe1558a71f 100644
>> --- a/arch/powerpc/kernel/eeh.c
>> +++ b/arch/powerpc/kernel/eeh.c
>> @@ -1628,8 +1628,8 @@ static ssize_t eeh_force_recover_write(struct file *filp,
>>         char buf[20];
>>         int ret;
>>
>> -       ret = simple_write_to_buffer(buf, sizeof(buf), ppos, user_buf, count);
>> -       if (!ret)
>> +       ret = simple_write_to_buffer(buf, sizeof(buf)-1, ppos, user_buf, count);
> 
> We should probably be zeroing the buffer. Reading to sizeof(buf) - 1
> is done in a few places to guarantee that the string is nul
> terminated, but without the preceeding memset() that isn't actually
> guaranteed.

Yes, the buffer should be zeroed out first. I have included memset() in Patch v2.

> 
>> +       if (ret <= 0)
>>                 return -EFAULT;
> 
> EFAULT is supposed to be returned when the user supplies a buffer to
> write(2) which is outside their address space. I figured letting the
> sscanf() in the next step fail if the user passes writes a zero-length
> buffer and returning EINVAL made more sense. That said, the exact
> semantics around zero length writes are pretty handwavy so I guess
> this isn't wrong, but I don't think it's better either.
> 

simple_write_to_buffer may return negative value on fail.
So, -EFAULT should be return in case of negative return value. 
The conditional (!ret) was not sufficient to catch negative return value.