Received-SPF: pass (google.com: domain of linux-kernel+bounces-167837-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 3 May 2024 16:36:06 +0100
From: Daniel Golle <daniel@makrotopia.org>
To: INAGAKI Hiroshi <musashino.open@gmail.com>
Cc: axboe@kernel.dk, yang.yang29@zte.com, justinstitt@google.com,
	xu.panda@zte.com.cn, linux-block@vger.kernel.org,
	linux-kernel@vger.kernel.org, Naohiro Aota <naota@elisp.net>
Subject: Re: [PATCH] block: fix and simplify blkdevparts= cmdline parsing
Message-ID: <ZjUEZtsJTaUt-5Yj@makrotopia.org>
References: <20240421074005.565-1-musashino.open@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240421074005.565-1-musashino.open@gmail.com>

On Sun, Apr 21, 2024 at 04:39:52PM +0900, INAGAKI Hiroshi wrote:
> Fix the cmdline parsing of the "blkdevparts=" parameter using strsep(),
> which makes the code simpler.
> 
> Before commit 146afeb235cc ("block: use strscpy() to instead of
> strncpy()"), we used a strncpy() to copy a block device name and partition
> names. The commit simply replaced a strncpy() and NULL termination with
> a strscpy(). It did not update calculations of length passed to strscpy().
> While the length passed to strncpy() is just a length of valid characters
> without NULL termination ('\0'), strscpy() takes it as a length of the
> destination buffer, including a NULL termination.
> 
> Since the source buffer is not necessarily NULL terminated, the current
> code copies "length - 1" characters and puts a NULL character in the
> destination buffer. It replaces the last character with NULL and breaks
> the parsing.
> 
> As an example, that buffer will be passed to parse_parts() and breaks
> parsing sub-partitions due to the missing ')' at the end, like the
> following.
> 
> example (Check Point V-80 & OpenWrt):
> 
> - Linux Kernel 6.6
> 
>   [    0.000000] Kernel command line: console=ttyS0,115200 earlycon=uart8250,mmio32,0xf0512000 crashkernel=30M mvpp2x.queue_mode=1 blkdevparts=mmcblk1:48M@10M(kernel-1),1M(dtb-1),720M(rootfs-1),48M(kernel-2),1M(dtb-2),720M(rootfs-2),300M(default_sw),650M(logs),1M(preset_cfg),1M(adsl),-(storage) maxcpus=4
>   ...
>   [    0.884016] mmc1: new HS200 MMC card at address 0001
>   [    0.889951] mmcblk1: mmc1:0001 004GA0 3.69 GiB
>   [    0.895043] cmdline partition format is invalid.
>   [    0.895704]  mmcblk1: p1
>   [    0.903447] mmcblk1boot0: mmc1:0001 004GA0 2.00 MiB
>   [    0.908667] mmcblk1boot1: mmc1:0001 004GA0 2.00 MiB
>   [    0.913765] mmcblk1rpmb: mmc1:0001 004GA0 512 KiB, chardev (248:0)
> 
>   1. "48M@10M(kernel-1),..." is passed to strscpy() with length=17
>      from parse_parts()
>   2. strscpy() returns -E2BIG and the destination buffer has
>      "48M@10M(kernel-1\0"
>   3. "48M@10M(kernel-1\0" is passed to parse_subpart()
>   4. parse_subpart() fails to find ')' when parsing a partition name,
>      and returns error
> 
> - Linux Kernel 6.1
> 
>   [    0.000000] Kernel command line: console=ttyS0,115200 earlycon=uart8250,mmio32,0xf0512000 crashkernel=30M mvpp2x.queue_mode=1 blkdevparts=mmcblk1:48M@10M(kernel-1),1M(dtb-1),720M(rootfs-1),48M(kernel-2),1M(dtb-2),720M(rootfs-2),300M(default_sw),650M(logs),1M(preset_cfg),1M(adsl),-(storage) maxcpus=4
>   ...
>   [    0.953142] mmc1: new HS200 MMC card at address 0001
>   [    0.959114] mmcblk1: mmc1:0001 004GA0 3.69 GiB
>   [    0.964259]  mmcblk1: p1(kernel-1) p2(dtb-1) p3(rootfs-1) p4(kernel-2) p5(dtb-2) 6(rootfs-2) p7(default_sw) p8(logs) p9(preset_cfg) p10(adsl) p11(storage)
>   [    0.979174] mmcblk1boot0: mmc1:0001 004GA0 2.00 MiB
>   [    0.984674] mmcblk1boot1: mmc1:0001 004GA0 2.00 MiB
>   [    0.989926] mmcblk1rpmb: mmc1:0001 004GA0 512 KiB, chardev (248:0
> 
> By the way, strscpy() takes a length of destination buffer and it is
> often confusing when copying characters with a specified length. Using
> strsep() helps to separate the string by the specified character. Then,
> we can use strscpy() naturally with the size of the destination buffer.
> 
> Separating the string on the fly is also useful to omit the redundant
> string copy, reducing memory usage and improve the code readability.
> 
> Fixes: 146afeb235cc ("block: use strscpy() to instead of strncpy()")
> Suggested-by: Naohiro Aota <naota@elisp.net>
> Signed-off-by: INAGAKI Hiroshi <musashino.open@gmail.com>

Reviewed-by: Daniel Golle <daniel@makrotopia.org>

> ---
>  block/partitions/cmdline.c | 49 ++++++++++----------------------------
>  1 file changed, 12 insertions(+), 37 deletions(-)
> 
> diff --git a/block/partitions/cmdline.c b/block/partitions/cmdline.c
> index c03bc105e575..152c85df92b2 100644
> --- a/block/partitions/cmdline.c
> +++ b/block/partitions/cmdline.c
> @@ -70,8 +70,8 @@ static int parse_subpart(struct cmdline_subpart **subpart, char *partdef)
>  	}
>  
>  	if (*partdef == '(') {
> -		int length;
> -		char *next = strchr(++partdef, ')');
> +		partdef++;
> +		char *next = strsep(&partdef, ")");
>  
>  		if (!next) {
>  			pr_warn("cmdline partition format is invalid.");
> @@ -79,11 +79,7 @@ static int parse_subpart(struct cmdline_subpart **subpart, char *partdef)
>  			goto fail;
>  		}
>  
> -		length = min_t(int, next - partdef,
> -			       sizeof(new_subpart->name) - 1);
> -		strscpy(new_subpart->name, partdef, length);
> -
> -		partdef = ++next;
> +		strscpy(new_subpart->name, next, sizeof(new_subpart->name));
>  	} else
>  		new_subpart->name[0] = '\0';
>  
> @@ -117,14 +113,12 @@ static void free_subpart(struct cmdline_parts *parts)
>  	}
>  }
>  
> -static int parse_parts(struct cmdline_parts **parts, const char *bdevdef)
> +static int parse_parts(struct cmdline_parts **parts, char *bdevdef)
>  {
>  	int ret = -EINVAL;
>  	char *next;
> -	int length;
>  	struct cmdline_subpart **next_subpart;
>  	struct cmdline_parts *newparts;
> -	char buf[BDEVNAME_SIZE + 32 + 4];
>  
>  	*parts = NULL;
>  
> @@ -132,28 +126,19 @@ static int parse_parts(struct cmdline_parts **parts, const char *bdevdef)
>  	if (!newparts)
>  		return -ENOMEM;
>  
> -	next = strchr(bdevdef, ':');
> +	next = strsep(&bdevdef, ":");
>  	if (!next) {
>  		pr_warn("cmdline partition has no block device.");
>  		goto fail;
>  	}
>  
> -	length = min_t(int, next - bdevdef, sizeof(newparts->name) - 1);
> -	strscpy(newparts->name, bdevdef, length);
> +	strscpy(newparts->name, next, sizeof(newparts->name));
>  	newparts->nr_subparts = 0;
>  
>  	next_subpart = &newparts->subpart;
>  
> -	while (next && *(++next)) {
> -		bdevdef = next;
> -		next = strchr(bdevdef, ',');
> -
> -		length = (!next) ? (sizeof(buf) - 1) :
> -			min_t(int, next - bdevdef, sizeof(buf) - 1);
> -
> -		strscpy(buf, bdevdef, length);
> -
> -		ret = parse_subpart(next_subpart, buf);
> +	while ((next = strsep(&bdevdef, ","))) {
> +		ret = parse_subpart(next_subpart, next);
>  		if (ret)
>  			goto fail;
>  
> @@ -199,24 +184,17 @@ static int cmdline_parts_parse(struct cmdline_parts **parts,
>  
>  	*parts = NULL;
>  
> -	next = pbuf = buf = kstrdup(cmdline, GFP_KERNEL);
> +	pbuf = buf = kstrdup(cmdline, GFP_KERNEL);
>  	if (!buf)
>  		return -ENOMEM;
>  
>  	next_parts = parts;
>  
> -	while (next && *pbuf) {
> -		next = strchr(pbuf, ';');
> -		if (next)
> -			*next = '\0';
> -
> -		ret = parse_parts(next_parts, pbuf);
> +	while ((next = strsep(&pbuf, ";"))) {
> +		ret = parse_parts(next_parts, next);
>  		if (ret)
>  			goto fail;
>  
> -		if (next)
> -			pbuf = ++next;
> -
>  		next_parts = &(*next_parts)->next_parts;
>  	}
>  
> @@ -250,7 +228,6 @@ static struct cmdline_parts *bdev_parts;
>  static int add_part(int slot, struct cmdline_subpart *subpart,
>  		struct parsed_partitions *state)
>  {
> -	int label_min;
>  	struct partition_meta_info *info;
>  	char tmp[sizeof(info->volname) + 4];
>  
> @@ -262,9 +239,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,
> -			  sizeof(subpart->name));
> -	strscpy(info->volname, subpart->name, label_min);
> +	strscpy(info->volname, subpart->name, sizeof(info->volname));
>  
>  	snprintf(tmp, sizeof(tmp), "(%s)", info->volname);
>  	strlcat(state->pp_buf, tmp, PAGE_SIZE);
> -- 
> 2.25.1
> 
>