[PATCH 0/2] state: deal gracefully with runtime bucket corruption

[PATCH 0/2] state: deal gracefully with runtime bucket corruption

[Ahmad Fatoum]

Series fixes two issues when buckets become corrupted during barebox
execution. Both issues are currently sorted out after one more reset,
so I'd suggest this goes into next despite being a fix.

Cheers,
Ahmad Fatoum (2):
  state: backend_storage: deal gracefully with runtime bucket corruption
  state: treat state with all-invalid buckets as dirty

 common/state/backend_storage.c | 2 ++
 common/state/state.c           | 7 +++----
 2 files changed, 5 insertions(+), 4 deletions(-)

-- 
2.25.1


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

[PATCH 1/2] state: backend_storage: deal gracefully with runtime bucket corruption

[Ahmad Fatoum]

Corrupting an already selected bucket and then reading it again will
crash barebox when it attempts the refresh:

  barebox$ state -l
  barebox$ mw -d /dev/eeprom0.state 0 0x42
  barebox$ state -l
  ERROR: state: No meta data header found
  state: Using bucket 1@0x00000040
  unable to handle NULL pointer dereference at address 0x00000000
  pc : [<4fe4f1ea>]    lr : [<4fe0bcb1>]
  sp : 4ffefd5c  ip : 00000000  fp : 2ff68f04
  r10: 4ffefdc8  r9 : 4b434d63  r8 : 30155f50
  r7 : 00000024  r6 : 2ff68b60  r5 : 2ff68e90  r4 : 00000000
  r3 : 00000024  r2 : 00000024  r1 : 30155f50  r0 : 00000000
  Flags: Nzcv  IRQs off  FIQs off  Mode SVC_32
  WARNING: [<4fe4f1ea>] (memcmp+0x14/0x1a) from [<4fe0bcb1>] (bucket_refresh.isra.0+0x4d/0x78)
  WARNING: [<4fe0bcb1>] (bucket_refresh.isra.0+0x4d/0x78) from [<4fe0be1d>] (state_storage_read+0xd1/0x104)
  WARNING: [<4fe0be1d>] (state_storage_read+0xd1/0x104) from [<4fe0a5bd>] (state_do_load+0x1d/0x78)
  WARNING: [<4fe0a5bd>] (state_do_load+0x1d/0x78) from [<4fe04137>] (execute_command+0x23/0x4c)

The memcmp called here is an optimization to skip I/O if the used bucket
and the one to be refreshed compare equal. Unfortunately, if the now
corrupt bucket was previously the used one, bucket->len will hold the
old value and we'll run into a NULL pointer dereference.

While this is quite inconvenient, it appears it doesn't affect
correctness: after the reset, the corrupt bucket will be refreshed
as expected.

Improve upon this by setting the length to zero when we are NULLing the
buffer. The zero length of the corrupted bucket will then compare unequal
to used_bucket->len in bucket_refresh() and ensure we will always refresh
the buffer if it becomes corrupted without an intermittent reset.

Fixes: 238008b4bd8f ("state: Drop cache bucket")
Cc: Enrico Jörns <ejo@pengutronix.de>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/state/backend_storage.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/state/backend_storage.c b/common/state/backend_storage.c
index fca887e93fa3..fe7e89e8fb39 100644
--- a/common/state/backend_storage.c
+++ b/common/state/backend_storage.c
@@ -192,6 +192,7 @@ int state_storage_read(struct state_backend_storage *storage,
 		/* Free buffer from the unused buckets */
 		free(bucket->buf);
 		bucket->buf = NULL;
+		bucket->len = 0;
 	}
 
 	/*
@@ -204,6 +205,7 @@ int state_storage_read(struct state_backend_storage *storage,
 
 	/* buffer from the used bucket is passed to the caller, do not free */
 	bucket_used->buf = NULL;
+	bucket_used->len = 0;
 
 	return 0;
 }
-- 
2.25.1


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

[PATCH 2/2] state: treat state with all-invalid buckets as dirty

[Ahmad Fatoum]

The state.dirty flag controls whether state_save will actually
persist state. It is cleared when we successfully load or save
state and set on writing a state parameter.

When the state however becomes corrupt during barebox runtime and
state.dirty == 0, reinitializing the state to defaults is quite
cumbersome:

1. We reset twice. After the first reset, the dirty flag is reset
   and before the second, state_save will reinitialize to defaults
2. We write any state variable and then run the state -s command

Both workarounds are quite obscure, improve the user experience
by having state -l set the dirty flag when it fails, so a subsequent
state -s may persist the default values to state.

Steps to reproduce:

  barebox$ state -l
  state: Using bucket 0@0x00000000
  barebox$ memcpy -s /dev/zero -d /dev/eeprom0.state 0 0 0x400
  barebox$ state -s
  barebox$ state -l
  ERROR: state: No meta data header found
  ERROR: state: No meta data header found
  ERROR: state: No meta data header found
  ERROR: state: Failed to find any valid state copy in any bucket
  ERROR: state: Failed to read state with format raw, -2
  state: No such file or directory

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/state/state.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/common/state/state.c b/common/state/state.c
index b168387eef0b..1822f37f3e29 100644
--- a/common/state/state.c
+++ b/common/state/state.c
@@ -94,7 +94,7 @@ out:
  */
 static int state_do_load(struct state *state, enum state_flags flags)
 {
-	void *buf;
+	void *buf = NULL;
 	ssize_t len;
 	int ret;
 
@@ -103,7 +103,7 @@ static int state_do_load(struct state *state, enum state_flags flags)
 	if (ret) {
 		dev_err(&state->dev, "Failed to read state with format %s, %d\n",
 			state->format->name, ret);
-		return ret;
+		goto out;
 	}
 
 	ret = state->format->unpack(state->format, state, buf, len);
@@ -114,9 +114,8 @@ static int state_do_load(struct state *state, enum state_flags flags)
 	}
 
 	state->init_from_defaults = 0;
-	state->dirty = 0;
-
 out:
+	state->dirty = !!ret; /* mark dirty on error */
 	free(buf);
 	return ret;
 }
-- 
2.25.1


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

Re: [PATCH 0/2] state: deal gracefully with runtime bucket corruption

[Sascha Hauer]

On Thu, Mar 05, 2020 at 08:40:30AM +0100, Ahmad Fatoum wrote:
> Series fixes two issues when buckets become corrupted during barebox
> execution. Both issues are currently sorted out after one more reset,
> so I'd suggest this goes into next despite being a fix.
> 
> Cheers,
> Ahmad Fatoum (2):
>   state: backend_storage: deal gracefully with runtime bucket corruption
>   state: treat state with all-invalid buckets as dirty
> 
>  common/state/backend_storage.c | 2 ++
>  common/state/state.c           | 7 +++----
>  2 files changed, 5 insertions(+), 4 deletions(-)

Applied, thanks

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
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