Re: [PATCH 1/2] common: state: Add property to protect existing data

[Sascha Hauer]

Sorry for replying only after reminding me. This patch is still
in my inbox as I have some problems with it. Anyway let's solve
them.

On Thu, Dec 14, 2017 at 04:51:36PM +0100, Daniel Schultz wrote:
> After an update to a newer barebox version with an enabled state
> framework, existing data in storage memories could be overwritten.
> 
> Add a new property to check in front of every write task, if the meta
> magic field only contains the magic number, zeros or ones.

What is the case you are trying to protect against? I assume partition
size changes and possibly old bootloaders with an old partitioning.
Could you explain?

Whatever solution we come up with should work for the circular backend
aswell.

What I am missing in this patch is a clear way how to overcome this
situation. For mtd it would be: erase the device and reset, but what
about devices that don't have to be erased? We might just need a -f aka
"I know what I am doing" parameter.

> 
> Signed-off-by: Daniel Schultz 
> ---
>  .../devicetree/bindings/barebox/barebox,state.rst |  3 +++
>  common/state/backend_bucket_direct.c  |  2 ++
>  common/state/backend_storage.c|  1 +
>  common/state/state.c  | 19 
> ++-
>  common/state/state.h  |  2 ++
>  5 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/barebox/barebox,state.rst 
> b/Documentation/devicetree/bindings/barebox/barebox,state.rst
> index cebb5f8..db5b041 100644
> --- a/Documentation/devicetree/bindings/barebox/barebox,state.rst
> +++ b/Documentation/devicetree/bindings/barebox/barebox,state.rst
> @@ -55,6 +55,9 @@ Optional Properties
>  * ``algo``: A HMAC algorithm used to detect manipulation of the data
>or header, sensible values follow this pattern ``hmac()``,
>e.g. ``hmac(sha256)``. Only available for the ``backend-type`` ``raw``.
> +* ``keep-previous-content``: Check if a the bucket meta magic field contains
> +  other data than the magic value. If so, the backend will not write the 
> state
> +  to prevent unconditionally overwrites of existing data.
>  
>  .. note:: For the ``backend-storage-type`` the keyword ``noncircular`` is 
> still
> supported as a fall back to an old storage format. Recommendation is to 
> not
> diff --git a/common/state/backend_bucket_direct.c 
> b/common/state/backend_bucket_direct.c
> index 958696e..9d6a337 100644
> --- a/common/state/backend_bucket_direct.c
> +++ b/common/state/backend_bucket_direct.c
> @@ -69,6 +69,8 @@ static int state_backend_bucket_direct_read(struct 
> state_backend_storage_bucket
>   if (meta.magic == direct_magic) {
>   read_len = meta.written_length;
>   } else {
> + if (meta.magic != ~0 && !!meta.magic)
> + bucket->wrong_magic = 1;

This only handles the direct backend, but not the circular backend.

>   if (!IS_ENABLED(CONFIG_STATE_BACKWARD_COMPATIBLE)) {
>   dev_err(direct->dev, "No meta data header found\n");
>   dev_dbg(direct->dev, "Enable backward compatibility or 
> increase stride size\n");
> diff --git a/common/state/backend_storage.c b/common/state/backend_storage.c
> index c6ebe86..f0a169d 100644
> --- a/common/state/backend_storage.c
> +++ b/common/state/backend_storage.c
> @@ -152,6 +152,7 @@ int state_storage_read(struct state_backend_storage 
> *storage,
>* one we want to use.
>*/
>   list_for_each_entry(bucket, >buckets, bucket_list) {
> + bucket->wrong_magic = 0;
>   ret = bucket->read(bucket, >buf, >len);
>   if (ret == -EUCLEAN)
>   bucket->needs_refresh = 1;
> diff --git a/common/state/state.c b/common/state/state.c
> index 6399bd3..e110542 100644
> --- a/common/state/state.c
> +++ b/common/state/state.c
> @@ -44,6 +44,9 @@ int state_save(struct state *state)
>   void *buf;
>   ssize_t len;
>   int ret;
> + struct state_backend_storage_bucket *bucket;
> + struct state_backend_storage *storage;
> + bool has_content;
>  
>   if (!state->dirty)
>   return 0;
> @@ -55,7 +58,18 @@ int state_save(struct state *state)
>   return ret;
>   }
>  
> - ret = state_storage_write(>storage, buf, len);
> + storage = >storage;
> + if (state->keep_prev_content) {
> + has_content = 0;

has_content is only used in this if(){}, can you move its declaration
here?

> + list_for_each_entry(bucket, >buckets, bucket_list)
> + has_content |= bucket->wrong_magic;
> + if (has_content) {
> + dev_dbg(>dev, "Found content on a backend.\n");

I think dev_dbg() is a bit quiet. This really deserves a warning saying
"Found foreign content on backend, won't overwrite.\n"

> + goto out;

You should initialize 

Re: [PATCH 1/2] common: state: Add property to protect existing data

[Daniel Schultz]

Hi,

just wanted to remind you to these patches.

Daniel


On 12/14/2017 04:51 PM, Daniel Schultz wrote:

After an update to a newer barebox version with an enabled state
framework, existing data in storage memories could be overwritten.

Add a new property to check in front of every write task, if the meta
magic field only contains the magic number, zeros or ones.

Signed-off-by: Daniel Schultz 
---
  .../devicetree/bindings/barebox/barebox,state.rst |  3 +++
  common/state/backend_bucket_direct.c  |  2 ++
  common/state/backend_storage.c|  1 +
  common/state/state.c  | 19 ++-
  common/state/state.h  |  2 ++
  5 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/barebox/barebox,state.rst 
b/Documentation/devicetree/bindings/barebox/barebox,state.rst
index cebb5f8..db5b041 100644
--- a/Documentation/devicetree/bindings/barebox/barebox,state.rst
+++ b/Documentation/devicetree/bindings/barebox/barebox,state.rst
@@ -55,6 +55,9 @@ Optional Properties
  * ``algo``: A HMAC algorithm used to detect manipulation of the data
or header, sensible values follow this pattern ``hmac()``,
e.g. ``hmac(sha256)``. Only available for the ``backend-type`` ``raw``.
+* ``keep-previous-content``: Check if a the bucket meta magic field contains
+  other data than the magic value. If so, the backend will not write the state
+  to prevent unconditionally overwrites of existing data.
  
  .. note:: For the ``backend-storage-type`` the keyword ``noncircular`` is still

 supported as a fall back to an old storage format. Recommendation is to not
diff --git a/common/state/backend_bucket_direct.c 
b/common/state/backend_bucket_direct.c
index 958696e..9d6a337 100644
--- a/common/state/backend_bucket_direct.c
+++ b/common/state/backend_bucket_direct.c
@@ -69,6 +69,8 @@ static int state_backend_bucket_direct_read(struct 
state_backend_storage_bucket
if (meta.magic == direct_magic) {
read_len = meta.written_length;
} else {
+   if (meta.magic != ~0 && !!meta.magic)
+   bucket->wrong_magic = 1;
if (!IS_ENABLED(CONFIG_STATE_BACKWARD_COMPATIBLE)) {
dev_err(direct->dev, "No meta data header found\n");
dev_dbg(direct->dev, "Enable backward compatibility or 
increase stride size\n");
diff --git a/common/state/backend_storage.c b/common/state/backend_storage.c
index c6ebe86..f0a169d 100644
--- a/common/state/backend_storage.c
+++ b/common/state/backend_storage.c
@@ -152,6 +152,7 @@ int state_storage_read(struct state_backend_storage 
*storage,
 * one we want to use.
 */
list_for_each_entry(bucket, >buckets, bucket_list) {
+   bucket->wrong_magic = 0;
ret = bucket->read(bucket, >buf, >len);
if (ret == -EUCLEAN)
bucket->needs_refresh = 1;
diff --git a/common/state/state.c b/common/state/state.c
index 6399bd3..e110542 100644
--- a/common/state/state.c
+++ b/common/state/state.c
@@ -44,6 +44,9 @@ int state_save(struct state *state)
void *buf;
ssize_t len;
int ret;
+   struct state_backend_storage_bucket *bucket;
+   struct state_backend_storage *storage;
+   bool has_content;
  
  	if (!state->dirty)

return 0;
@@ -55,7 +58,18 @@ int state_save(struct state *state)
return ret;
}
  
-	ret = state_storage_write(>storage, buf, len);

+   storage = >storage;
+   if (state->keep_prev_content) {
+   has_content = 0;
+   list_for_each_entry(bucket, >buckets, bucket_list)
+   has_content |= bucket->wrong_magic;
+   if (has_content) {
+   dev_dbg(>dev, "Found content on a backend.\n");
+   goto out;
+   }
+   }
+
+   ret = state_storage_write(storage, buf, len);
if (ret) {
dev_err(>dev, "Failed to write packed state, %d\n", ret);
goto out;
@@ -605,6 +619,9 @@ struct state *state_new_from_node(struct device_node *node, 
char *path,
}
}
  
+	state->keep_prev_content = of_property_read_bool(node,

+   
"keep-previous-content");
+
state->backend_path = xstrdup(path);
  
  	ret = of_property_read_string(node, "backend-type", _type);

diff --git a/common/state/state.h b/common/state/state.h
index fcc6b9f..a411b96 100644
--- a/common/state/state.h
+++ b/common/state/state.h
@@ -46,6 +46,7 @@ struct state_backend_storage_bucket {
void *buf;
ssize_t len;
bool needs_refresh;
+   bool wrong_magic;
  };
  
  /**

@@ -105,6 +106,7 @@ struct state {
char *of_path;
const char *name;
uint32_t 

[PATCH 1/2] common: state: Add property to protect existing data

[Daniel Schultz]

After an update to a newer barebox version with an enabled state
framework, existing data in storage memories could be overwritten.

Add a new property to check in front of every write task, if the meta
magic field only contains the magic number, zeros or ones.

Signed-off-by: Daniel Schultz 
---
 .../devicetree/bindings/barebox/barebox,state.rst |  3 +++
 common/state/backend_bucket_direct.c  |  2 ++
 common/state/backend_storage.c|  1 +
 common/state/state.c  | 19 ++-
 common/state/state.h  |  2 ++
 5 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/barebox/barebox,state.rst 
b/Documentation/devicetree/bindings/barebox/barebox,state.rst
index cebb5f8..db5b041 100644
--- a/Documentation/devicetree/bindings/barebox/barebox,state.rst
+++ b/Documentation/devicetree/bindings/barebox/barebox,state.rst
@@ -55,6 +55,9 @@ Optional Properties
 * ``algo``: A HMAC algorithm used to detect manipulation of the data
   or header, sensible values follow this pattern ``hmac()``,
   e.g. ``hmac(sha256)``. Only available for the ``backend-type`` ``raw``.
+* ``keep-previous-content``: Check if a the bucket meta magic field contains
+  other data than the magic value. If so, the backend will not write the state
+  to prevent unconditionally overwrites of existing data.
 
 .. note:: For the ``backend-storage-type`` the keyword ``noncircular`` is still
supported as a fall back to an old storage format. Recommendation is to not
diff --git a/common/state/backend_bucket_direct.c 
b/common/state/backend_bucket_direct.c
index 958696e..9d6a337 100644
--- a/common/state/backend_bucket_direct.c
+++ b/common/state/backend_bucket_direct.c
@@ -69,6 +69,8 @@ static int state_backend_bucket_direct_read(struct 
state_backend_storage_bucket
if (meta.magic == direct_magic) {
read_len = meta.written_length;
} else {
+   if (meta.magic != ~0 && !!meta.magic)
+   bucket->wrong_magic = 1;
if (!IS_ENABLED(CONFIG_STATE_BACKWARD_COMPATIBLE)) {
dev_err(direct->dev, "No meta data header found\n");
dev_dbg(direct->dev, "Enable backward compatibility or 
increase stride size\n");
diff --git a/common/state/backend_storage.c b/common/state/backend_storage.c
index c6ebe86..f0a169d 100644
--- a/common/state/backend_storage.c
+++ b/common/state/backend_storage.c
@@ -152,6 +152,7 @@ int state_storage_read(struct state_backend_storage 
*storage,
 * one we want to use.
 */
list_for_each_entry(bucket, >buckets, bucket_list) {
+   bucket->wrong_magic = 0;
ret = bucket->read(bucket, >buf, >len);
if (ret == -EUCLEAN)
bucket->needs_refresh = 1;
diff --git a/common/state/state.c b/common/state/state.c
index 6399bd3..e110542 100644
--- a/common/state/state.c
+++ b/common/state/state.c
@@ -44,6 +44,9 @@ int state_save(struct state *state)
void *buf;
ssize_t len;
int ret;
+   struct state_backend_storage_bucket *bucket;
+   struct state_backend_storage *storage;
+   bool has_content;
 
if (!state->dirty)
return 0;
@@ -55,7 +58,18 @@ int state_save(struct state *state)
return ret;
}
 
-   ret = state_storage_write(>storage, buf, len);
+   storage = >storage;
+   if (state->keep_prev_content) {
+   has_content = 0;
+   list_for_each_entry(bucket, >buckets, bucket_list)
+   has_content |= bucket->wrong_magic;
+   if (has_content) {
+   dev_dbg(>dev, "Found content on a backend.\n");
+   goto out;
+   }
+   }
+
+   ret = state_storage_write(storage, buf, len);
if (ret) {
dev_err(>dev, "Failed to write packed state, %d\n", ret);
goto out;
@@ -605,6 +619,9 @@ struct state *state_new_from_node(struct device_node *node, 
char *path,
}
}
 
+   state->keep_prev_content = of_property_read_bool(node,
+   
"keep-previous-content");
+
state->backend_path = xstrdup(path);
 
ret = of_property_read_string(node, "backend-type", _type);
diff --git a/common/state/state.h b/common/state/state.h
index fcc6b9f..a411b96 100644
--- a/common/state/state.h
+++ b/common/state/state.h
@@ -46,6 +46,7 @@ struct state_backend_storage_bucket {
void *buf;
ssize_t len;
bool needs_refresh;
+   bool wrong_magic;
 };
 
 /**
@@ -105,6 +106,7 @@ struct state {
char *of_path;
const char *name;
uint32_t magic;
+   bool keep_prev_content;
 
struct list_head variables; /* Sorted list of variables */