Received-SPF: pass (google.com: domain of linux-kernel+bounces-13983-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <c7e29d85-277d-46ae-87ae-bb77dd423652@gmail.com>
Date: Tue, 2 Jan 2024 10:31:27 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] block: fix length of strscpy()
Content-Language: en-US
To: David Laight <David.Laight@ACULAB.COM>, "axboe@kernel.dk"
 <axboe@kernel.dk>
Cc: "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
 "stable@vger.kernel.org" <stable@vger.kernel.org>
References: <20240101175051.38479-2-pugokushin@gmail.com>
 <ed0b9dd45fca4f6e910a9e1ffa756180@AcuMS.aculab.com>
From: Guoxin Pu <pugokushin@gmail.com>
In-Reply-To: <ed0b9dd45fca4f6e910a9e1ffa756180@AcuMS.aculab.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

Thank you for the review. Sorry if this is the duplicated reply, as I 
didn't configure my mail client to send text-only message and the 
previous mail was rejected by the list.

On 02/01/2024 05:47, David Laight wrote:
>> @@ -79,8 +79,8 @@ static int parse_subpart(struct cmdline_subpart **subpart, char *partdef)
>>   			goto fail;
>>   		}
>>
>> -		length = min_t(int, next - partdef,
>> -			       sizeof(new_subpart->name) - 1);
>> +		length = min_t(int, next - partdef + 1,
>> +			       sizeof(new_subpart->name));
>>   		strscpy(new_subpart->name, partdef, length);
> Shouldn't that be a memcpy() with the original length?
> Since it looks as though there is something equivalent to:
> 		next = strchr(partdef, ',');
> just above?
> Maybe with:
> 		new_subpart->name[length] = '\0';
> if the target isn't zero filled (which the strncpy() probably
> relied on.)

Yes that would be better. But since I'm fixing the issue caused by the 
mentioned commit, which was an accepted change to use strscpy instead of 
strncpy and seems a part of a series of changes to do that, I think 
there might be a reason the maintainers preferred strscpy over strncpy 
over memcpy? Otherwise we could just revert that commit and keep using 
the original strncpy + setting NULL method, and then potentially swap 
strncpy with memcpy.

On 02/01/2024 05:47, David Laight wrote:

 > Same

Same.

On 02/01/2024 05:47, David Laight wrote:
>> @@ -262,7 +262,7 @@ static int add_part(int slot, struct cmdline_subpart *subpart,
>>
>>   	info = &state->parts[slot].info;
>>
>> -	label_min = min_t(int, sizeof(info->volname) - 1,
>> +	label_min = min_t(int, sizeof(info->volname),
>>   			  sizeof(subpart->name));
>>   	strscpy(info->volname, subpart->name, label_min);
> WTF?
> That only makes any sense if subpart->name might not be '\0'
> terminated - which strncpy() would have handled fine (with the -1).
> Otherwise what is wrong with:
> 	strscpy(info->volname, subpart->name, sizeof (info->volname));
>
> 	David

Yes, there is no need to calculate label_min here. We could remove int 
label_min altogether in this function and use a single line of strscpy.