Adding Iterator and rename functions #148

MarcusJGStreets · 2024-01-16T11:15:58Z

Added in PS storage initially.
If thee is need for them in ITS, the same additions can be made there

athoelke

Need to clarify the conceptual behavior of psa_ps_rename() - 'replace-item' or 'move-content'?

doc/storage/api/api.rst

athoelke

The iterator API needs:

Clarification about where the iteration state is managed (the API design partly dictates this, but is not explicit)
Would benefit from a usage example
Needs clarity on interleaved update operations
Should probably use an implementation-defined type, rather than a byte-array for the context

doc/storage/api/api.rst

doc/storage/about.rst

athoelke · 2024-02-02T16:59:28Z

doc/storage/api/api.rst

@@ -9,7 +9,7 @@ Status codes

 The |API| uses the status code definitions that are shared with the other PSA Certified APIs.

-The following elements are defined in :file:`psa/error.h` from :cite-title:`PSA-STAT` (previously defined in :cite:`PSA-FFM`):
+The following elements are defined in :file:`psa/error.h` from :cite-title:`PSA-STAT` (previously defined in :cite:`PSA-FF-M`):


This is another artifact of the initial file content not being fully synchronised with upstream.

This has been broken again. Related to the previous commit about build errors which also undoes changes made in main in January,

doc/storage/api/api.rst

athoelke · 2024-02-02T18:14:28Z

doc/storage/api/api.rst

+
+   A caller may read the contents of any `uid` with ``psa_ps_get()`` or write with ``psa_ps_set()`` or ``psa_ps_set_extended()`` without invalidating the iteration context. 
+
+   A caller may create a `uid` with ``psa_ps_set()`` or ``psa_ps_create()`` without invalidating the iteration context, provided the `uid` does *NOT* match the filter. However, if the `uid` matches the filter then any later call to `psa_ps_iterator_next()` fails with `PSA_ERROR_DATA_CORRUPT`.


These constraints may be challenging to implement. Does this requirement place an arbitrary cost on the implementation?

For example, if an application starts an iteration, and then creates 20 new data items, how does the implementation determine if it must report PSA_ERROR_DATA_CORRUPT? It does not have information about the iterator (all state in iterator object in caller), and storing all of the UIDs modified since last iteration, for checking on next ietration, leads to arbitrary volatile storage requirements.

doc/storage/api/api.rst

athoelke

Largely there I think in terms of the API design.

I think we might need to decide how to deal with the optionality of the new APIs. (and maybe review what is said in the spec about psa_ps_create() and psa_ps_set_extended()). This includes whether an implementation that claims to be v1.1 (presumably what this update will be), is permitted to not support the iterator API. If so, how should we articulate this in the API specification, consistently across all affected APIs.

The example needs to be improved to be recognisably, and mostly compilable, C code:

Use the xref format for the code block (results in hyperlinked API elements)
Data types
Bracketing
Extracting checking the return code from psa_ps_iterator_next()

US English spelling please.

athoelke · 2024-02-15T14:39:38Z

doc/storage/api/api.rst


+    .. summary::
+        An implementation-defined opaque structure containing the context for an iterator. 
+        The structure MUST contain all all the state required by the iterator. 


From this line onward, this should form the content of the Description (not the summary). This needs a blank line and then indent this and the following lines to the level of the body of the struct directive (i.e. aligned with .. summary::).

athoelke · 2024-02-15T14:39:58Z

doc/storage/api/api.rst

+        The structure is initilaised by the `ps_iterator_start` function. 
+        It is modified by the `ps_iterator_next` function. 
+
+        the caller can discard or reuse the iterator object once it has finished using it. This can be before, or after, the iterator has reached the end of the iteration.


Capitalisation of 'The'

athoelke · 2024-02-15T14:40:27Z

doc/storage/api/api.rst

+
+        the caller can discard or reuse the iterator object once it has finished using it. This can be before, or after, the iterator has reached the end of the iteration.
+
+        The header file is only required to define this structure if PSA_STORAGE_SUPPORT_ITERATION is `True`


Presumably this is based on whether the macro is defined (as specified below) or not, rather than having a "value of 'True'"?

athoelke · 2024-02-15T14:42:48Z

doc/storage/api/api.rst

+
+    Flag indicating that `psa_ps_iterator_init()` and `psa_ps_iterator_next()` are supported.
+
+.. macro:: PSA_STORAGE_ITERATOR_CTX__SIZE


This definition is no longer required for the API.

athoelke · 2024-02-15T14:46:05Z

doc/storage/api/api.rst

@@ -379,6 +415,9 @@ These definitions must be defined in the header file :file:`psa/protected_storag
        * The ``uid`` is ``0``.

        * The operation failed because caller cannot access some or all of the memory in the range [``p_data``, ``p_data + data_length - 1``].
+
+        * the uid exists and ``data_length`` is greater then ```capacity`` 


This addition contradicts the description. It is always permitted to overwrite an item created with psa_ps_create() using psa_ps_set() - the new constraint requires that the item must be deleted before it is replaced.

athoelke · 2024-02-15T15:00:17Z

doc/storage/api/api.rst

+
+   An iterator MUST return all data objects whose `uid` matches the filter that are extant when the filter was created, unless these are deleted or renamed before the iteration would return them, or the caller stops before all matching objects have been returned. 
+
+   A caller may delete a `uid` with ``psa_ps_remove(uid)`` without invalidating the iteration context. the iterator MUST never return a `uid` that has been deleted. However, if the caller is multi-threaded it ia possible another thread may delete a `uid`. 


That last sentence probably isn't required - except perhaps as a note. It does not constrain the implementation, but is just a reminder to multi-threaded app developers about the risk of race conditions.

athoelke · 2024-02-15T15:03:40Z

doc/storage/api/api.rst

+
+   An iterator MUST return all data objects whose `uid` matches the filter that are extant when the filter was created, unless these are deleted or renamed before the iteration would return them, or the caller stops before all matching objects have been returned. 
+
+   A caller may delete a `uid` with ``psa_ps_remove(uid)`` without invalidating the iteration context. the iterator MUST never return a `uid` that has been deleted. However, if the caller is multi-threaded it ia possible another thread may delete a `uid`. 


For inline code that includes API identifiers, using the :code: role enables the identifiers to be hyperlinked.

For inline code that is not an API element, use double back-quotes.

For inline code that is only an API element, use single back-quotes.

If in doubt, use the :code: role which will hyperlink if possible, otherwise render as code.

Suggested change

A caller may delete a `uid` with ``psa_ps_remove(uid)`` without invalidating the iteration context. the iterator MUST never return a `uid` that has been deleted. However, if the caller is multi-threaded it ia possible another thread may delete a `uid`.

A caller may delete a ``uid`` with :code:`psa_ps_remove(uid)` without invalidating the iteration context. the iterator MUST never return a ``uid`` that has been deleted. However, if the caller is multi-threaded it ia possible another thread may delete a ``uid``.

athoelke · 2024-02-15T15:04:06Z

doc/storage/api/api.rst

+
+   A caller may delete a `uid` with ``psa_ps_remove(uid)`` without invalidating the iteration context. the iterator MUST never return a `uid` that has been deleted. However, if the caller is multi-threaded it ia possible another thread may delete a `uid`. 
+
+   A caller may read the contents of any `uid` with ``psa_ps_get()`` or write with ``psa_ps_set()`` or ``psa_ps_set_extended()`` without invalidating the iteration context. 


Suggested change

A caller may read the contents of any `uid` with ``psa_ps_get()`` or write with ``psa_ps_set()`` or ``psa_ps_set_extended()`` without invalidating the iteration context.

A caller may read the contents of any ``uid`` with `psa_ps_get()` or write with `psa_ps_set()` or `psa_ps_set_extended()` without invalidating the iteration context.

etc... (there are other instances)

athoelke · 2024-02-15T15:05:13Z

doc/storage/api/api.rst

+
+   A caller may read the contents of any `uid` with ``psa_ps_get()`` or write with ``psa_ps_set()`` or ``psa_ps_set_extended()`` without invalidating the iteration context. 
+
+   A caller may create a `uid` with ``psa_ps_set()`` or ``psa_ps_create()`` without invalidating the iteration context. However, the iterator is NOT guaranteed to return the new object, `uid`, the behaviour is dependent on both implementation and identity. In particular, the iterator is not expected to return `uid` if the iteration is already past the point at which it would naturally be returned. 


I think 'For example' works better than 'In particular' - as this is not normative text.

athoelke · 2024-02-15T15:05:41Z

doc/storage/api/api.rst

+
+   A caller may call `psa_ps_rename(uid, uid_new)` without invalidating the iteration context. The iterator must not return `uid`. The iterator is not guaranteed to return `uid_new`, the behaviour is dependent on both implementation and identity.  
+
+   The following code snippet uses a linked list to store the matching files before iterating over that list and removing them. 


This comment is out of date as the linked list has been removed.

athoelke · 2024-04-15T11:32:16Z

The merge of main back into this branch has no effect because Git believes that the relevant changes have previously been merged, and this branch has then made changes after that merge.

To repair the inadvertent 'revert' changes to some of the source files will require more effort.

You could add a new commit to change them back to the current content on main. That isn't ideal in terms of history, but it works.
Otherwise you need to find the commits in the current branch that introduce those changes, and then edit those commits to no longer do so. An interactive rebase can make that process a bit easier.
Or start with the first option, then use an interactive rebase to move the 'fix' commit immediately after the commit that makes the incorrect change, and mark it as 'fixup' in the interactive rebase file.

Signed-off-by: Marcus Streets <marcus.streets@arm.com>

Adding SP800-30 to references. Signed-off-by: Marcus Streets <marcus.streets@arm.com>

Signed-off-by: Marcus Streets <marcus.streets@arm.com>

MarcusJGStreets · 2024-04-19T12:16:43Z

Added in PS storage initially. If thee is need for them in ITS, the same additions can be made there

Now added to ITS

Signed-off-by: Marcus Streets <marcus.streets@arm.com>

athoelke

I have some more changes to work through... but here are an initial set to consider.

athoelke · 2024-04-25T08:51:58Z

doc/storage/about.rst

@@ -1,4 +1,4 @@
-.. SPDX-FileCopyrightText: Copyright 2018-2019, 2022-2024 Arm Limited and/or its affiliates <open-source-office@arm.com>
+.. SPDX-FileCopyrightText: Copyright 2018-2019, 2022-2023 Arm Limited and/or its affiliates <open-source-office@arm.com>


This entire commit is undoing changes in 'main' from earlier in the year. None of these changes should be in this PR.

This branch somehow manages to revert changes to the main branch. Most of the changes in this file should not be in the PR.

The only update that is expected from this PR is the addition of the 1.1.0 release information

athoelke · 2024-04-25T08:52:47Z

doc/storage/api/api.rst


    .. summary::
        An implementation-defined opaque structure containing the context for an iterator.
        The structure MUST contain all all the state required by the iterator.
        That is, further state MUST NOT be retained by the implementation.

-        The structure is initilaised by the `ps_iterator_start()` function.
-        It is modified by the `ps_iterator_next()` function.
+        The structure is initilaised by the ``ps_iterator_start()`` function.


Presumably this should refer to the psa_its_iterator_start() function?

athoelke · 2024-04-25T08:52:58Z

doc/storage/api/api.rst

-        The structure is initilaised by the `ps_iterator_start()` function.
-        It is modified by the `ps_iterator_next()` function.
+        The structure is initilaised by the ``ps_iterator_start()`` function.
+        It is modified by the ``ps_iterator_next()`` function.


Presumably this should refer to the psa_its_iterator_next() function?

athoelke · 2024-04-25T08:56:37Z

doc/storage/api/api.rst

+        The structure MUST contain all all the state required by the iterator.
+        That is, further state MUST NOT be retained by the implementation.
+
+        The structure is initilaised by the ``ps_iterator_start()`` function.


Presumably this should refer to the psa_ps_iterator_start() function?

athoelke · 2024-04-25T08:56:54Z

doc/storage/api/api.rst

+        That is, further state MUST NOT be retained by the implementation.
+
+        The structure is initilaised by the ``ps_iterator_start()`` function.
+        It is modified by the ``ps_iterator_next()`` function.


Presumably this should refer to the psa_ps_iterator_next() function?

athoelke · 2024-04-25T09:45:54Z

doc/storage/api/api.rst

+
+       This function must be fully defined if `PSA_STORAGE_SUPPORT_ITERATION` is true.
+
+       If `PSA_STORAGE_SUPPORT_ITERATION` is false, then this function SHALL always return ``PSA_ERROR_NOT_SUPPORTED``


PSA_STORAGE_SUPPORT_ITERATION is a macro defining a flag value.

The condition 'XXX is false' is not clear for such a macro:

Does it mean that the implementation could define XXX to be 'false' (i.e. 0)? - contrary to the specification definition.

Does it mean that the implementation can NOT define XXX? - causing a compilation error is XXX is referenced in C code, but evaluating as 0 if used in pre-processor logic such as #if XXX.

Does it mean return value returned by psa_ps_get_support() does not include the XXX flag?

The spec should be clear if the API elements can be elided entirely from the header file, but still claim to be API v1.1; or if the iteration API elements must be defined (for a v1.1 implementation) but can always return PSA_ERROR_NOT_SUPPORTED (perhaps as an inline implementation).

athoelke · 2024-04-25T09:48:30Z

doc/storage/api/api.rst

@@ -379,6 +551,9 @@ These definitions must be defined in the header file :file:`psa/protected_storag
        * The ``uid`` is ``0``.

        * The operation failed because caller cannot access some or all of the memory in the range [``p_data``, ``p_data + data_length - 1``].
+
+        * the uid exists and ``data_length`` is greater then ```capacity``


This contradicts the established behavior for this API (replacing the entire item), and also the text in the API description (line 568 below).

To operate on content only, within allocated capacity, use psa_ps_set_extended().

athoelke · 2024-04-25T09:56:15Z

doc/storage/api/api.rst

+.. macro:: PSA_STORAGE_SUPPORT_ITERATION
+    (1u << 2)
+
+    Flag indicating that `psa_its_iterator_start()`, `psa_its_iterator_next()`  `psa_ps_iterator_start` and `psa_ps_iterator_next` are supported.


Issue: the current API for evaluating the supported functions is psa_ps_get_support() - which does not suggest that it provides information about support for ITS functionality.

Would a parallel psa_its_get_support() API be valuable? - this also permits an implementation to independently provide support for rename/iteration depending on the storage class. This would work for runtime detection of support - if compile time detection is required then a separate flag is needed for ITS and PS support of these functions.

athoelke · 2024-04-25T09:57:08Z

doc/storage/api/api.rst


-    This function is optional. Consult the platform documentation to determine if it is implemented or perform a call to `psa_ps_get_support()`. This function must be implemented if `psa_ps_get_support()` returns `PSA_STORAGE_SUPPORT_SET_EXTENDED`.
+      This function is optional. Consult the platform documentation to determine if it is implemented or perform a call to ``psa_ps_get_support()``. This function must be implemented if ``psa_ps_get_support()`` returns ``PSA_STORAGE_SUPPORT_SET_EXTENDED``.


Added leading space which will upset the formatting.

athoelke · 2024-04-25T10:01:01Z

doc/storage/api/api.rst

+
+
+   .. param::  psa_its_storage_iterator_t* context
+       A pointer to a context for this iterator. The pointer may be NULL. This is set to a new value on success and is undefined on error. The content of the iterator is implementation defined.


The optional NULL value looks odd, as it would seem that the entire purpose of starting an iteration would be to iterate.

There is no mention of how the function behaves if no iterator object is provided, or why a caller would ever want to do this. If there is a use case (I can think of a couple) where this is a better approach than provided an iterator object, it would benefit from an explanation and/or code example in the description.

The same applies to psa_ps_iterator_start().

Signed-off-by: Marcus Streets <marcus.streets@arm.com>

MarcusJGStreets · 2024-07-18T15:16:32Z

Updates with clarifications that iterator start returns a pointer to a new context.
And split the support iterator macro into ITS and PS for compile time detection.

athoelke

A number of minor nits, and a handful of more significant issues to resolve:

The about.txt file is still broken (reverts changes in main)
How do we want to handle optionality in the API (see specific comments for issue & options)
Iterator description for ITS should be in the start function, not next.
Structures should also be type[def]s. This is easy to add - and we should probably do the same for the legacy psa_storage_info_t structure (this will be backwards compatible).
The usage example code is not quite valid C, and the introductory text is out of date.

athoelke · 2024-07-19T14:36:42Z

doc/storage/about.rst

@@ -1,4 +1,4 @@
-.. SPDX-FileCopyrightText: Copyright 2018-2019, 2022-2024 Arm Limited and/or its affiliates <open-source-office@arm.com>
+.. SPDX-FileCopyrightText: Copyright 2018-2019, 2022-2023 Arm Limited and/or its affiliates <open-source-office@arm.com>


This branch somehow manages to revert changes to the main branch. Most of the changes in this file should not be in the PR.

athoelke · 2024-07-19T14:37:13Z

doc/storage/about.rst

@@ -1,4 +1,4 @@
-.. SPDX-FileCopyrightText: Copyright 2018-2019, 2022-2024 Arm Limited and/or its affiliates <open-source-office@arm.com>
+.. SPDX-FileCopyrightText: Copyright 2018-2019, 2022-2023 Arm Limited and/or its affiliates <open-source-office@arm.com>


The only update that is expected from this PR is the addition of the 1.1.0 release information

athoelke · 2024-07-19T14:38:29Z

doc/storage/api/api.rst

+
+    Flag indicating that `psa_its_iterator_start()` and  `psa_its_iterator_next()` are supported.
+
+.. struct:: psa_its_storage_iterator_t


This should probably be annotated with the :type: option so that this identifier is both a typedef and a struct tag:

Suggested change

.. struct:: psa_its_storage_iterator_t

.. struct:: psa_its_storage_iterator_t

:type:

I've changed my mind: as this is a fully implementation-defined type, we should probably use the same pattern as the Crypto API for such types, and just use a typedef in this specification. For example, see the definition of psa_key_attributes_t. This gives the implementation full flexibility in the definition of the type.

To do this, replace:

.. struct:: psa_its_storage_iterator_t :type:

with

.. typedef:: /* implementation-defined type */ psa_its_storage_iterator_t

Likewise for the PS iterator type.

In the Crypto API, we then refer to these as 'objects' rather than 'structures' - i.e. use 'iterator object' in this spec when describing a variable of this type.

doc/storage/api/api.rst

Signed-off-by: Marcus Streets <marcus.streets@arm.com>

Added Rename to ITS Added "Introduced in 1.1" where relevant Updated words on optionality. A few other minor fixes as noted by Andrew. Signed-off-by: Marcus Streets <marcus.streets@arm.com>

athoelke

Only some minor issues remain

athoelke · 2024-07-26T11:39:50Z

doc/storage/api/api.rst

@@ -305,14 +332,195 @@ These definitions must be defined in the header file :file:`psa/internal_trusted
    .. retval:: PSA_ERROR_STORAGE_FAILURE
        The operation failed because the physical storage has failed (Fatal error).

-    Deletes the data from internal storage.
+   Introduced in Version 1.1. 


psa_its_remove() has been in the API since 1.0. Lines 335-337 are not required here.

athoelke · 2024-07-26T11:40:48Z

doc/storage/api/api.rst

+
+        * ``uid`` is ``0``.
+        * ``uid_new`` is ``0``
+        * the ``psa_storage_rename_flags_t`` has a value set other than `PSA_STORAGE_FLAG_REPLACE`


This should be about the rename_flags parameter, not the type.

Suggested change

* the ``psa_storage_rename_flags_t`` has a value set other than `PSA_STORAGE_FLAG_REPLACE`

* ``rename_flags`` has a value set other than `PSA_STORAGE_FLAG_REPLACE`.

athoelke · 2024-07-26T11:41:22Z

doc/storage/api/api.rst

+
+   Introduced in Version 1.1. 
+
+   This function takes an iterator that was returned by the `psa_its_iterator_start` function and searches the storage for the next ``uid`` that matches the filters defined in that function call. If a ``uid`` matching the filter exists, the function updates the ``result`` and ``context`` parameters and retursn PSA_SUCCESS. If not it reruns an error.        


Spelling: 'reruns' -> 'returns'

athoelke · 2024-07-26T11:41:35Z

doc/storage/api/api.rst

+
+   Introduced in Version 1.1. 
+
+   This function takes an iterator that was returned by the `psa_ps_iterator_start` function and searches the storage for the next ``uid`` that matches the filters defined in that function call. If a ``uid`` matching the filter exists, the function updates the ``result`` and ``context`` parameters and retursn PSA_SUCCESS. If not it reruns an error. 


Spelling: 'reruns' -> 'returns'

athoelke · 2024-07-26T11:42:00Z

doc/storage/api/api.rst

+
+        * ``uid`` is ``0``.
+        * ``uid_new`` is ``0``
+        * the ``psa_storage_rename_flags_t`` has a value set other than `PSA_STORAGE_FLAG_REPLACE`


Suggested change

* the ``psa_storage_rename_flags_t`` has a value set other than `PSA_STORAGE_FLAG_REPLACE`

* ``rename_flags`` has a value set other than `PSA_STORAGE_FLAG_REPLACE`.

athoelke · 2024-07-26T11:49:25Z

doc/storage/api/api.rst

+
+    Flag indicating that `psa_its_iterator_start()` and  `psa_its_iterator_next()` are supported.
+
+.. struct:: psa_its_storage_iterator_t


I've changed my mind: as this is a fully implementation-defined type, we should probably use the same pattern as the Crypto API for such types, and just use a typedef in this specification. For example, see the definition of psa_key_attributes_t. This gives the implementation full flexibility in the definition of the type.

To do this, replace:

.. struct:: psa_its_storage_iterator_t :type:

with

.. typedef:: /* implementation-defined type */ psa_its_storage_iterator_t

Likewise for the PS iterator type.

In the Crypto API, we then refer to these as 'objects' rather than 'structures' - i.e. use 'iterator object' in this spec when describing a variable of this type.

athoelke · 2024-07-26T11:52:50Z

While working through the issues for this addition: I wonder if we should improve the text for the optional 1.0 functions psa_ps_create() and psa_ps_set_extended()?

The current wording is not clear whether an implementation that does not support these functions must provide definitions of them in a header file (perhaps with inline implementations), and so ensure that an application that references these functions will compile?

What does TF-M do?

MarcusJGStreets requested a review from athoelke January 16, 2024 11:16

MarcusJGStreets marked this pull request as draft January 16, 2024 11:16

MarcusJGStreets requested a review from gilles-peskine-arm January 16, 2024 11:16

athoelke changed the title ~~Adding Iterator and remane functions~~ Adding Iterator and rename functions Jan 22, 2024

athoelke requested changes Jan 23, 2024

View reviewed changes

doc/storage/api/api.rst Outdated Show resolved Hide resolved

doc/storage/api/api.rst Show resolved Hide resolved

doc/storage/api/api.rst Outdated Show resolved Hide resolved

doc/storage/api/api.rst Outdated Show resolved Hide resolved

doc/storage/api/api.rst Outdated Show resolved Hide resolved

athoelke requested changes Jan 25, 2024

View reviewed changes

athoelke reviewed Jan 31, 2024

View reviewed changes