diff mbox series

[03/16] arm: stm32mp: reset to default environment when serial# change

Message ID 20200331180330.3.I8f6df6d28ce5b4b601ced711af3699d95e1576fb@changeid
State Rejected
Delegated to: Patrick Delaunay
Headers show
Series [01/16] arm: stm32mp: update dependency for STM32_ETZPC | expand

Commit Message

Patrick Delaunay March 31, 2020, 4:04 p.m. UTC
Serial number is first checked and, in case of mismatch, all
environment variables are reset to their default value.

This patch allows to detect that environment is saved in a removable
device, as a SD card, and reused on a other board, potentially with
incompatible variables.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 arch/arm/mach-stm32mp/cpu.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

Comments

Patrice CHOTARD April 1, 2020, 7:22 a.m. UTC | #1
Hi Patrick   

On 3/31/20 6:04 PM, Patrick Delaunay wrote:
> Serial number is first checked and, in case of mismatch, all
> environment variables are reset to their default value.
>
> This patch allows to detect that environment is saved in a removable
> device, as a SD card, and reused on a other board, potentially with
> incompatible variables.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
>  arch/arm/mach-stm32mp/cpu.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
> index 9aa5794334..365c2aa4f7 100644
> --- a/arch/arm/mach-stm32mp/cpu.c
> +++ b/arch/arm/mach-stm32mp/cpu.c
> @@ -511,8 +511,9 @@ __weak int setup_mac_address(void)
>  		return -EINVAL;
>  	}
>  	pr_debug("OTP MAC address = %pM\n", enetaddr);
> -	ret = !eth_env_set_enetaddr("ethaddr", enetaddr);
> -	if (!ret)
> +
> +	ret = eth_env_set_enetaddr("ethaddr", enetaddr);
> +	if (ret)
>  		pr_err("Failed to set mac address %pM from OTP: %d\n",
>  		       enetaddr, ret);
>  #endif
> @@ -522,13 +523,13 @@ __weak int setup_mac_address(void)
>  
>  static int setup_serial_number(void)
>  {
> +	char *serial_env;
>  	char serial_string[25];
>  	u32 otp[3] = {0, 0, 0 };
>  	struct udevice *dev;
>  	int ret;
>  
> -	if (env_get("serial#"))
> -		return 0;
> +	serial_env = env_get("serial#");
>  
>  	ret = uclass_get_device_by_driver(UCLASS_MISC,
>  					  DM_GET_DRIVER(stm32mp_bsec),
> @@ -542,6 +543,15 @@ static int setup_serial_number(void)
>  		return ret;
>  
>  	sprintf(serial_string, "%08X%08X%08X", otp[0], otp[1], otp[2]);
> +
> +	if (serial_env) {
> +		if (!strcmp(serial_string, serial_env))
> +			return 0;
> +		/* For invalid enviromnent (serial# change), reset to default */
> +		env_set_default("serial number mismatch", 0);
> +	}
> +
> +	/* save serial number */
>  	env_set("serial#", serial_string);
>  
>  	return 0;
> @@ -549,9 +559,9 @@ static int setup_serial_number(void)
>  
>  int arch_misc_init(void)
>  {
> +	setup_serial_number();
>  	setup_boot_mode();
>  	setup_mac_address();
> -	setup_serial_number();
>  
>  	return 0;
>  }

Reviewed-by: Patrice Chotard <patrice.chotard@st.com>

Thanks
Wolfgang Denk April 1, 2020, 11:18 a.m. UTC | #2
Dear Patrick,

In message <20200331180330.3.I8f6df6d28ce5b4b601ced711af3699d95e1576fb@changeid> you wrote:
> Serial number is first checked and, in case of mismatch, all
> environment variables are reset to their default value.
>
> This patch allows to detect that environment is saved in a removable
> device, as a SD card, and reused on a other board, potentially with
> incompatible variables.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
>  arch/arm/mach-stm32mp/cpu.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
> index 9aa5794334..365c2aa4f7 100644
> --- a/arch/arm/mach-stm32mp/cpu.c
> +++ b/arch/arm/mach-stm32mp/cpu.c
> @@ -511,8 +511,9 @@ __weak int setup_mac_address(void)
>  		return -EINVAL;
>  	}
>  	pr_debug("OTP MAC address = %pM\n", enetaddr);
> -	ret = !eth_env_set_enetaddr("ethaddr", enetaddr);
> -	if (!ret)
> +
> +	ret = eth_env_set_enetaddr("ethaddr", enetaddr);
> +	if (ret)
>  		pr_err("Failed to set mac address %pM from OTP: %d\n",
>  		       enetaddr, ret);

This is an unrelated and totally undocumented change.  Please split
into separate commit.


> +
> +	if (serial_env) {
> +		if (!strcmp(serial_string, serial_env))
> +			return 0;
> +		/* For invalid enviromnent (serial# change), reset to default */
> +		env_set_default("serial number mismatch", 0);
> +	}

Resetting the enviropnment to the defaults means you drop all
setting sa user may have made.  This is a very bad move - as a user
I find such things completely unacceptable.  If I make any changes,
they must never ever be killed without my explicit confirmation.

I strongly advice against such a method. Please drop that.

Best regards,

Wolfgang Denk
Patrick Delaunay April 7, 2020, 2:31 p.m. UTC | #3
Dear Wolfgang,

> From: Wolfgang Denk <wd@denx.de>
> Sent: mercredi 1 avril 2020 13:19
> 
> Dear Patrick,
> 
> In message
> <20200331180330.3.I8f6df6d28ce5b4b601ced711af3699d95e1576fb@changeid>
> you wrote:
> > Serial number is first checked and, in case of mismatch, all
> > environment variables are reset to their default value.
> >
> > This patch allows to detect that environment is saved in a removable
> > device, as a SD card, and reused on a other board, potentially with
> > incompatible variables.
> >
> > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> > ---
> >
> >  arch/arm/mach-stm32mp/cpu.c | 20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
> > index 9aa5794334..365c2aa4f7 100644
> > --- a/arch/arm/mach-stm32mp/cpu.c
> > +++ b/arch/arm/mach-stm32mp/cpu.c
> > @@ -511,8 +511,9 @@ __weak int setup_mac_address(void)
> >  		return -EINVAL;
> >  	}
> >  	pr_debug("OTP MAC address = %pM\n", enetaddr);
> > -	ret = !eth_env_set_enetaddr("ethaddr", enetaddr);
> > -	if (!ret)
> > +
> > +	ret = eth_env_set_enetaddr("ethaddr", enetaddr);
> > +	if (ret)
> >  		pr_err("Failed to set mac address %pM from OTP: %d\n",
> >  		       enetaddr, ret);
> 
> This is an unrelated and totally undocumented change.  Please split into separate
> commit.

Yes, sorry.

Done in http://patchwork.ozlabs.org/project/uboot/list/?series=169021
" arm: stm32mp: cleanup test on eth_env_set_enetaddr result"

> 
> > +
> > +	if (serial_env) {
> > +		if (!strcmp(serial_string, serial_env))
> > +			return 0;
> > +		/* For invalid enviromnent (serial# change), reset to default */
> > +		env_set_default("serial number mismatch", 0);
> > +	}
> 
> Resetting the enviropnment to the defaults means you drop all setting sa user
> may have made.  This is a very bad move - as a user I find such things
> completely unacceptable.  If I make any changes, they must never ever be killed
> without my explicit confirmation.
> 
> I strongly advice against such a method. Please drop that.

Understood, I drops this patch....

I introduce this behavior after a internal request and to avoid issues during tests:

Some users use the same SD card (same rootfs) on several boards (EV1 and DK2 for example)
and the U-Boot environment is saved on this SD card. 

When an user updates U-Boot binary to use this SD card on other board but not erase the
environment file (save in ext4 file in bootfs partition), the boot can fail because the
environment is not compatible between the 2 boards.

This patch try to avoid this issue when I detect that the removable device (as SD card) is used on a
a different board (it is detected when saved serial number with different the OTP).

But I admit that can be strange/unacceptable for end-user, particularly it is only change of board
(DK2#1 => DK2#2) : the environment tuned one board #2 can't be used on board#2.

I make too many assumption on user usage and this patch can't be acceptable in arch 
(common for all board based on STM32MP15x).
 
> Best regards,
> 
> Wolfgang Denk
> 
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "You
> ain't experienced..." "Well, nor are you." "That's true. But the point is ... the point is
> ... the point is we've been not experienced for a lot longer than you. We've got  a
> lot  of  experience  of  not
> having any experience."           - Terry Pratchett, _Witches Abroad_
Wolfgang Denk April 7, 2020, 4:22 p.m. UTC | #4
Dear Patrick,

In message <60273317e5704581bef81c4beb28ad75@SFHDAG6NODE3.st.com> you wrote:
> 
> > I strongly advice against such a method. Please drop that.
> 
> Understood, I drops this patch....

Thanks.

> I introduce this behavior after a internal request and to avoid issues during tests:
> 
> Some users use the same SD card (same rootfs) on several boards (EV1 and DK2 for example)
> and the U-Boot environment is saved on this SD card. 

OK.

> When an user updates U-Boot binary to use this SD card on other board but not erase the
> environment file (save in ext4 file in bootfs partition), the boot can fail because the
> environment is not compatible between the 2 boards.

Well, why would that fail on another board but not on the one that
is currently in use?  Where is the U-Boot image you are booting
from?   Not on the SDCard, too?

Well, then this is a design problem (or you may even call it a
design bug).  It has always been a bad idea to use a fixed structure
binary format if there are chances that provider (the env storage)
and consumer (U-Boot) may get out of sync.

The binary blob environment format (checksum, eventually redundancy
flag, date with a fixed total size) is inherently only compatible
with other U-Boot versions as long as redundancy and size settings
are kept the same.

If you cannot garantee this, you should use a different storage
format - for example as plain text file.  Of course you pay for
added compatibility across U-Boot configurations with the price of
reduced security (lack of checksum).

But then, normally you do not change redundancy or environment size
between U-Boot versions, so all this should be a theroretical
problem only?


> This patch try to avoid this issue when I detect that the removable device (as SD card) is used on a
> a different board (it is detected when saved serial number with different the OTP).

You see this only as a risk - try to see it as a chance, too.
Imagine a user is trying to copy environment between systems.  Why
would you want to stop him?

Any incompatibility issues can reliably be avoided when using binary
blob format, as then you will get a checksum error, and the
incorrect copy is ignored.

> I make too many assumption on user usage and this patch can't be acceptable in arch 
> (common for all board based on STM32MP15x).

See signature below :-)

Best regards,

Wolfgang Denk
diff mbox series

Patch

diff --git a/arch/arm/mach-stm32mp/cpu.c b/arch/arm/mach-stm32mp/cpu.c
index 9aa5794334..365c2aa4f7 100644
--- a/arch/arm/mach-stm32mp/cpu.c
+++ b/arch/arm/mach-stm32mp/cpu.c
@@ -511,8 +511,9 @@  __weak int setup_mac_address(void)
 		return -EINVAL;
 	}
 	pr_debug("OTP MAC address = %pM\n", enetaddr);
-	ret = !eth_env_set_enetaddr("ethaddr", enetaddr);
-	if (!ret)
+
+	ret = eth_env_set_enetaddr("ethaddr", enetaddr);
+	if (ret)
 		pr_err("Failed to set mac address %pM from OTP: %d\n",
 		       enetaddr, ret);
 #endif
@@ -522,13 +523,13 @@  __weak int setup_mac_address(void)
 
 static int setup_serial_number(void)
 {
+	char *serial_env;
 	char serial_string[25];
 	u32 otp[3] = {0, 0, 0 };
 	struct udevice *dev;
 	int ret;
 
-	if (env_get("serial#"))
-		return 0;
+	serial_env = env_get("serial#");
 
 	ret = uclass_get_device_by_driver(UCLASS_MISC,
 					  DM_GET_DRIVER(stm32mp_bsec),
@@ -542,6 +543,15 @@  static int setup_serial_number(void)
 		return ret;
 
 	sprintf(serial_string, "%08X%08X%08X", otp[0], otp[1], otp[2]);
+
+	if (serial_env) {
+		if (!strcmp(serial_string, serial_env))
+			return 0;
+		/* For invalid enviromnent (serial# change), reset to default */
+		env_set_default("serial number mismatch", 0);
+	}
+
+	/* save serial number */
 	env_set("serial#", serial_string);
 
 	return 0;
@@ -549,9 +559,9 @@  static int setup_serial_number(void)
 
 int arch_misc_init(void)
 {
+	setup_serial_number();
 	setup_boot_mode();
 	setup_mac_address();
-	setup_serial_number();
 
 	return 0;
 }