From dc5100e6ba686fafd5570ce6d972383f047c7313 Mon Sep 17 00:00:00 2001 From: Ahmad Fatoum Date: Thu, 5 Mar 2020 08:40:31 +0100 Subject: state: backend_storage: deal gracefully with runtime bucket corruption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Ahmad Fatoum Signed-off-by: Sascha Hauer --- common/state/backend_storage.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'common') diff --git a/common/state/backend_storage.c b/common/state/backend_storage.c index fca887e93f..fe7e89e8fb 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; } -- cgit v1.2.3 From f41e5160c8618455064a4ff4227105010cd56aaa Mon Sep 17 00:00:00 2001 From: Ahmad Fatoum Date: Thu, 5 Mar 2020 08:40:32 +0100 Subject: state: treat state with all-invalid buckets as dirty 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 Signed-off-by: Sascha Hauer --- common/state/state.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'common') diff --git a/common/state/state.c b/common/state/state.c index b168387eef..1822f37f3e 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; } -- cgit v1.2.3