[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <66fa81f2.050a0220.3358eb.f750@mx.google.com>
Date: Mon, 30 Sep 2024 12:48:11 +0200
From: Christian Marangi <ansuelsmth@...il.com>
To: Rasmus Villemoes <linux@...musvillemoes.dk>
Cc: Jens Axboe <axboe@...nel.dk>, Jonathan Corbet <corbet@....net>,
Ulf Hansson <ulf.hansson@...aro.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
INAGAKI Hiroshi <musashino.open@...il.com>,
Daniel Golle <daniel@...rotopia.org>,
Christian Brauner <brauner@...nel.org>,
Al Viro <viro@...iv.linux.org.uk>, Jan Kara <jack@...e.cz>,
Li Lingfeng <lilingfeng3@...wei.com>,
Christian Heusel <christian@...sel.eu>, linux-block@...r.kernel.org,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-mmc@...r.kernel.org, devicetree@...r.kernel.org,
Miquel Raynal <miquel.raynal@...tlin.com>,
Lorenzo Bianconi <lorenzo@...nel.org>, upstream@...oha.com
Subject: Re: [PATCH v3 3/4] block: add support for partition table defined in
OF
On Mon, Sep 30, 2024 at 11:21:53AM +0200, Rasmus Villemoes wrote:
> Christian Marangi <ansuelsmth@...il.com> writes:
>
> > diff --git a/block/partitions/of.c b/block/partitions/of.c
> > new file mode 100644
> > index 000000000000..bc6200eb86b3
> > --- /dev/null
> > +++ b/block/partitions/of.c
> > @@ -0,0 +1,151 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/blkdev.h>
> > +#include <linux/major.h>
> > +#include <linux/of.h>
> > +#include "check.h"
> > +
> > +#define BOOT0_STR "boot0"
> > +#define BOOT1_STR "boot1"
> > +
> > +static struct device_node *get_partitions_node(struct device_node *disk_np,
> > + struct gendisk *disk)
> > +{
> > + const char *node_name = "partitions";
> > +
> > + /*
> > + * JEDEC specification 4.4 for eMMC introduced 3 additional partition
> > + * present on every eMMC. These additional partition are always hardcoded
> > + * from the eMMC driver as boot0, boot1 and rpmb. While rpmb is used to
> > + * store keys and exposed as a char device, the other 2 are exposed as
> > + * real separate disk with the boot0/1 appended to the disk name.
> > + *
> > + * Here we parse the disk_name in search for such suffix and select
> > + * the correct partition node.
> > + */
> > + if (disk->major == MMC_BLOCK_MAJOR) {
> > + const char *disk_name = disk->disk_name;
> > +
> > + if (!memcmp(disk_name + strlen(disk_name) - strlen(BOOT0_STR),
> > + BOOT0_STR, sizeof(BOOT0_STR)))
> > + node_name = "partitions-boot0";
>
> If strlen(disk_name) is less than 5 (and I don't know if that's actually
> possible), this well end up doing out-of-bounds access.
>
> We have a strstarts() helper, could you also add a strends() helper that
> handles this correctly? Something like
>
> /**
> * strends - does @str end with @suffix?
> * @str: string to examine
> * @suffix: suffix to look for.
> */
> static inline bool strends(const char *str, const char *suffix)
> {
> size_t n = strlen(str);
> size_t m = strlen(suffix);
> return n >= m && !memcmp(str + n - m, suffix, m);
> }
>
> [or name it str_has_suffix() or str_ends_with(), "strends" is not
> particularly readable, it's unfortunate that the existing strstarts is
> spelled like that].
>
Nice idea and thanks for checking the problem with the out-of-bounds
read.
Out of consistency with the unreadable strstarts I'm tempted to use
strends.
Since checking suffix of a string can't be something that unreal I
searched for the 3 function name and to my surprise all 3 suggested name
have a variant of the function statically defined hahaha.
To not pollute this series I will just introduce the helper but I will
add on my TODO list to convert the other function to make use of this
helper instead.
--
Ansuel
Powered by blists - more mailing lists