[PATCH] clk: fix NULL deref without OF node in debug message

[PATCH] clk: fix NULL deref without OF node in debug message

[matthias.schiffer]

From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

of_clk_add_provider() may be called for devices added via
add_generic_device(). There is no OF node in this case, leading to a crash
when debug logs are enabled.

This affects various i.MX CPUs, which add imx*-ccm clock devices using
add_generic_device().

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/clk/clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 24759b45b..6a2d8ad17 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -413,7 +413,7 @@ int of_clk_add_provider(struct device_node *np,
 	cp->get = clk_src_get;
 
 	list_add(&cp->link, &of_clk_providers);
-	pr_debug("Added clock from %s\n", np->full_name);
+	pr_debug("Added clock from %s\n", np ? np->full_name : "<none>");
 
 	return 0;
 }
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH] environment: do not attempt to erase devices with MTD_NO_ERASE

[matthias.schiffer]

From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

Devices like MRAM do not need to be erased; in fact, trying to do a partial
erase will fail with -EINVAL, as they don't have a proper erase block size
defined.

The State backend checks for the MTD_NO_ERASE flag before attempting to
erase a device. Add a similar check to envfs_save().

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 common/environment.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/common/environment.c b/common/environment.c
index 56a030eda..28deb3468 100644
--- a/common/environment.c
+++ b/common/environment.c
@@ -38,6 +38,8 @@
 #include <environment.h>
 #include <globalvar.h>
 #include <libfile.h>
+#include <asm-generic/ioctl.h>
+#include <linux/mtd/mtd-abi.h>
 #else
 # define errno_str(x) ("void")
 #define EXPORT_SYMBOL(x)
@@ -88,6 +90,17 @@ static int do_compare_file(const char *filename, const char *base)
 	return ret;
 }
 
+static inline int can_erase(int fd)
+{
+	struct mtd_info_user meminfo;
+	int ret = -ENODEV;
+
+	if (IS_ENABLED(CONFIG_MTD))
+		ret = ioctl(fd, MEMGETINFO, &meminfo);
+
+	return ret || !(meminfo.flags & MTD_NO_ERASE);
+}
+
 #else
 #define ERASE_SIZE_ALL 0
 static inline int protect(int fd, size_t count, unsigned long offset, int prot)
@@ -104,6 +117,11 @@ static int do_compare_file(const char *filename, const char *base)
 {
 	return 1;
 }
+
+static inline int can_erase(int fd)
+{
+	return 0;
+}
 #endif
 
 static int file_action(const char *filename, struct stat *statbuf,
@@ -334,12 +352,16 @@ int envfs_save(const char *filename, const char *dirname, unsigned flags)
 		goto out;
 	}
 
-	ret = erase(envfd, ERASE_SIZE_ALL, 0);
+	if (can_erase(envfd)) {
+		ret = erase(envfd, ERASE_SIZE_ALL, 0);
 
-	/* ENOSYS and EOPNOTSUPP aren't errors here, many devices don't need it */
-	if (ret && errno != ENOSYS && errno != EOPNOTSUPP) {
-		printf("could not erase %s: %s\n", filename, errno_str());
-		goto out;
+		/* ENOSYS and EOPNOTSUPP aren't errors here, many devices don't
+		 * need it */
+		if (ret && errno != ENOSYS && errno != EOPNOTSUPP) {
+			printf("could not erase %s: %s\n", filename,
+			       errno_str());
+			goto out;
+		}
 	}
 
 	size += sizeof(struct envfs_super);
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] environment: do not attempt to erase devices with MTD_NO_ERASE

[Sascha Hauer]

Hi Matthias,

On Tue, Nov 20, 2018 at 10:40:34AM +0100, matthias.schiffer@ew.tq-group.com wrote:
> From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> 
> Devices like MRAM do not need to be erased; in fact, trying to do a partial
> erase will fail with -EINVAL, as they don't have a proper erase block size
> defined.

Where does this -EINVAL come from? Wouldn't it be an option to check for
the MTD_NO_ERASE flag mtd_op_erase() and return -EOPNOTSUPP there?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] environment: do not attempt to erase devices with MTD_NO_ERASE

[Matthias Schiffer]

On Wed, 2018-11-21 at 08:56 +0100, Sascha Hauer wrote:
> Hi Matthias,
> 
> On Tue, Nov 20, 2018 at 10:40:34AM +0100, 
> matthias.schiffer@ew.tq-group.com wrote:
> > From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > 
> > Devices like MRAM do not need to be erased; in fact, trying to do a
> > partial
> > erase will fail with -EINVAL, as they don't have a proper erase
> > block size
> > defined.
> 
> Where does this -EINVAL come from? Wouldn't it be an option to check
> for
> the MTD_NO_ERASE flag mtd_op_erase() and return -EOPNOTSUPP there?
> 
> Sascha
> 

Hmm, what are the expected semantics of MTD_NO_ERASE - "does not need
erase" or "does not support erase"? According to code comments, it's
the former.

The MRAM I'm working with reports an erase size equal to its total
size:

barebox:/ devinfo m25p0
Parameters:
  erasesize: 131072 (type: uint32)
  oobsize: 0 (type: uint32)
  partitions: 128k(barebox-environment) (type: string)
  size: 131072 (type: uint64)
  writesize: 1 (type: uint32)

This matches what Linux reports, but it seems to me that this MRAM does
not actually implement it: When I try to erase the whole flash (from
Linux or Barebox), it does return success, but the data is unchanged.
When I configure the environment to span the whole flash, envfs_save()
is working fine without my patch, as the erase is successful.

The problems start when I try to partition the MRAM, as envfs_save()
will now try to erase a block smaller than the erasesize, leading to
-EINVAL.

I'm not sure if making mtd_op_erase() check for MTD_NO_ERASE is the
correct solution - it would obviously be fine for my MRAM, as erase is
a noop anyways, but I have no idea if there are other devices that set
the flag, but do support erase (and just don't need it for normal write
operations).

Anyways, I have no problem with implementing the fix that way if we
decide that these are the semantics we want.

Matthias


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] environment: do not attempt to erase devices with MTD_NO_ERASE

[Sascha Hauer]

On Thu, Nov 22, 2018 at 11:19:30AM +0100, Matthias Schiffer wrote:
> On Wed, 2018-11-21 at 08:56 +0100, Sascha Hauer wrote:
> > Hi Matthias,
> > 
> > On Tue, Nov 20, 2018 at 10:40:34AM +0100, 
> > matthias.schiffer@ew.tq-group.com wrote:
> > > From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > > 
> > > Devices like MRAM do not need to be erased; in fact, trying to do a
> > > partial
> > > erase will fail with -EINVAL, as they don't have a proper erase
> > > block size
> > > defined.
> > 
> > Where does this -EINVAL come from? Wouldn't it be an option to check
> > for
> > the MTD_NO_ERASE flag mtd_op_erase() and return -EOPNOTSUPP there?
> > 
> > Sascha
> > 
> 
> Hmm, what are the expected semantics of MTD_NO_ERASE - "does not need
> erase" or "does not support erase"? According to code comments, it's
> the former.
> 
> The MRAM I'm working with reports an erase size equal to its total
> size:
> 
> barebox:/ devinfo m25p0
> Parameters:
>   erasesize: 131072 (type: uint32)
>   oobsize: 0 (type: uint32)
>   partitions: 128k(barebox-environment) (type: string)
>   size: 131072 (type: uint64)
>   writesize: 1 (type: uint32)
> 
> This matches what Linux reports, but it seems to me that this MRAM does
> not actually implement it: When I try to erase the whole flash (from
> Linux or Barebox), it does return success, but the data is unchanged.

So actually your device does not support erase. I'd say "does not
support erase" is the correct semantics for MTD_NO_ERASE, at least I
can't think of any devices that support an optional erase operation.

> When I configure the environment to span the whole flash, envfs_save()
> is working fine without my patch, as the erase is successful.
> 
> The problems start when I try to partition the MRAM, as envfs_save()
> will now try to erase a block smaller than the erasesize, leading to
> -EINVAL.

The MRAM specifies an erasesize of 128KiB and then afterwards an erase
operation is a no-op. This seems wrong, at least inconsistent. This
should be discussed on the Linux mtd list.

For barebox I think it's fine to return -EOPNOTSUPP for devices which
have the MTD_NO_ERASE flag. At least for your device this return value
really is correct.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2] mtd: return EOPNOTSUPP when attempting to erase an MTD_NO_ERASE device

[Matthias Schiffer]

At least some MTD_NO_ERASE devices like MRAM do not specify a sensible
erase block size; instead, erasesize is equal to the whole flash size. This
leads to an EINVAL return from mtd_erase_align() whenever a partial erase
is attempted.

At least on the MRAM I tested, a full flash erase did not return an error,
but it did not have any effect on the flash either. As erase seems to be
entirely unsupported on this class of devices, and it is not necessary
anyways, it's better to return early with EOPNOTSUPP.

This fixes envfs_save() on a partitioned MRAM.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---

v2: handle in mtd_op_erase() instead of envfs_save()

 drivers/mtd/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/core.c b/drivers/mtd/core.c
index 56e85b3d8..d3cbe502f 100644
--- a/drivers/mtd/core.c
+++ b/drivers/mtd/core.c
@@ -178,6 +178,9 @@ static int mtd_op_erase(struct cdev *cdev, loff_t count, loff_t offset)
 	loff_t addr;
 	int ret;
 
+	if (mtd->flags & MTD_NO_ERASE)
+		return -EOPNOTSUPP;
+
 	ret = mtd_erase_align(mtd, &count, &offset);
 	if (ret)
 		return ret;
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2] mtd: return EOPNOTSUPP when attempting to erase an MTD_NO_ERASE device

[Sascha Hauer]

On Fri, Nov 23, 2018 at 03:50:15PM +0100, Matthias Schiffer wrote:
> At least some MTD_NO_ERASE devices like MRAM do not specify a sensible
> erase block size; instead, erasesize is equal to the whole flash size. This
> leads to an EINVAL return from mtd_erase_align() whenever a partial erase
> is attempted.
> 
> At least on the MRAM I tested, a full flash erase did not return an error,
> but it did not have any effect on the flash either. As erase seems to be
> entirely unsupported on this class of devices, and it is not necessary
> anyways, it's better to return early with EOPNOTSUPP.
> 
> This fixes envfs_save() on a partitioned MRAM.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---

Applied, thanks

Sascha

> 
> v2: handle in mtd_op_erase() instead of envfs_save()
> 
>  drivers/mtd/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mtd/core.c b/drivers/mtd/core.c
> index 56e85b3d8..d3cbe502f 100644
> --- a/drivers/mtd/core.c
> +++ b/drivers/mtd/core.c
> @@ -178,6 +178,9 @@ static int mtd_op_erase(struct cdev *cdev, loff_t count, loff_t offset)
>  	loff_t addr;
>  	int ret;
>  
> +	if (mtd->flags & MTD_NO_ERASE)
> +		return -EOPNOTSUPP;
> +
>  	ret = mtd_erase_align(mtd, &count, &offset);
>  	if (ret)
>  		return ret;
> -- 
> 2.17.1
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH] FIT: look for hash-1 as well as hash@1 nodes in signature check

[matthias.schiffer]

From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

The examples in the U-boot docs use "hash-N" as the name for hash nodes.
It seems "hash@N" was used instead at some point during the development of
the FIT format and "hash-N" is more correct; support for "hash@N" is
preserved for backward compatibility.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 common/image-fit.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/common/image-fit.c b/common/image-fit.c
index dfd1fa02c..356c1ae5d 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -392,7 +392,9 @@ static int fit_verify_hash(struct fit_handle *handle, struct device_node *image,
 		ret = -EINVAL;
 	}
 
-	hash = of_get_child_by_name(image, "hash@1");
+	hash = of_get_child_by_name(image, "hash-1");
+	if (!hash)
+		hash = of_get_child_by_name(image, "hash@1");
 	if (!hash) {
 		if (ret)
 			pr_err("image %s does not have hashes\n",
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] FIT: look for hash-1 as well as hash@1 nodes in signature check

[Sascha Hauer]

On Tue, Nov 20, 2018 at 10:40:35AM +0100, matthias.schiffer@ew.tq-group.com wrote:
> From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> 
> The examples in the U-boot docs use "hash-N" as the name for hash nodes.
> It seems "hash@N" was used instead at some point during the development of
> the FIT format and "hash-N" is more correct; support for "hash@N" is
> preserved for backward compatibility.

Indeed. dtc lately throughs warnings when a node with a '@' in it
doesn't have a reg property.

> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  common/image-fit.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/common/image-fit.c b/common/image-fit.c
> index dfd1fa02c..356c1ae5d 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -392,7 +392,9 @@ static int fit_verify_hash(struct fit_handle *handle, struct device_node *image,
>  		ret = -EINVAL;
>  	}
>  
> -	hash = of_get_child_by_name(image, "hash@1");
> +	hash = of_get_child_by_name(image, "hash-1");
> +	if (!hash)
> +		hash = of_get_child_by_name(image, "hash@1");

A few lines below we have the same with:

	sig_node = of_get_child_by_name(image, "signature@1");

Care to fix this aswell in this patch?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2] FIT: support hash-1/signature-1 nodes in signature check

[Matthias Schiffer]

The examples in the U-boot docs use "hash-N" and "signature-N" as the names
for hash/signature nodes. It seems "@N" was used instead at some point
during the development of the FIT format and "-N" is more correct (in fact,
dtc throws warnings when using "@N" without a reg attribute). Support for
the "@N" node names is preserved for backward compatibility.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---

v2: add signature-1 in addition to hash-1

 common/image-fit.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/common/image-fit.c b/common/image-fit.c
index dfd1fa02c..87a55b7e2 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -392,7 +392,9 @@ static int fit_verify_hash(struct fit_handle *handle, struct device_node *image,
 		ret = -EINVAL;
 	}
 
-	hash = of_get_child_by_name(image, "hash@1");
+	hash = of_get_child_by_name(image, "hash-1");
+	if (!hash)
+		hash = of_get_child_by_name(image, "hash@1");
 	if (!hash) {
 		if (ret)
 			pr_err("image %s does not have hashes\n",
@@ -468,7 +470,9 @@ static int fit_image_verify_signature(struct fit_handle *handle,
 		ret = -EINVAL;
 	}
 
-	sig_node = of_get_child_by_name(image, "signature@1");
+	sig_node = of_get_child_by_name(image, "signature-1");
+	if (!sig_node)
+		sig_node = of_get_child_by_name(image, "signature@1");
 	if (!sig_node) {
 		pr_err("Image %s has no signature\n", image->full_name);
 		return ret;
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] clk: fix NULL deref without OF node in debug message

[Sascha Hauer]

On Tue, Nov 20, 2018 at 10:40:33AM +0100, matthias.schiffer@ew.tq-group.com wrote:
> From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> 
> of_clk_add_provider() may be called for devices added via
> add_generic_device(). There is no OF node in this case, leading to a crash
> when debug logs are enabled.
> 
> This affects various i.MX CPUs, which add imx*-ccm clock devices using
> add_generic_device().
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  drivers/clk/clk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied this one and the third one for now.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox