Add API for flash write protection and readout protection functionality

[duda-patryk]

This PR extends flash API to expose flash write protection (persistent across reboots) and readout protection (temporary or permanent) functionality.

The flash API was extended by functions responsible for checking and changing WP and RDP state. Enabling RDP permanently is guarded by FLASH_ALLOW_PERMANENT_READOUT_PROTECTION option. Disabling RDP (which triggers mass erase) is guarded by FLASH_ALLOW_DISABLING_READOUT_PROTECTION.

Best regards,
Patryk

[duda-patryk]

Added RFC #53905

[duda-patryk]

Replaced flash API functions for WP, RDP with one "extended operations" function as discussed in #53905

[Laczen]

I am not yet convinced about the function parameters. Would it be clearer to pass instead:

device *dev, uint32_t code, const void *code_param

[duda-patryk]

I'm fine with one pointer instead of two. One note: we will need to have separate codes to get and set option (which is also fine). I would remove also const (so, const void *code_param will become void *code_data), because we need to pass data from get codes to the user.

Do you strongly prefer void * over uintptr_t *?

I will wait for comments from @de-nordic before changing this.

[Laczen]

I'm fine with one pointer instead of two. One note: we will need to have separate codes to get and set option (which is also fine). I would remove also const (so, const void *code_param will become void *code_data), because we need to pass data from get codes to the user.

I would pass the code_data pointer as a const void *, if it would be a "get" operation it would contain a pointer to where the data can be written (not a const) and a size that has been allocated for that. In a "set" operation it is also possible to pass the size. So the idea was not to use this directly but only after a cast to something containing a size and a buffer location.

Do you strongly prefer void * over uintptr_t *?

void * is used more than uintptr_t * in the code and make it more clear that it is expected to be casted.

I will wait for comments from @de-nordic before changing this.

OK

[duda-patryk]

So the idea was not to use this directly but only after a cast to something containing a size and a buffer location.

I'm not sure if this is better than passing pointer and size in a flash_ex_op call. From the user perspective, in order to get a write protect status I need to allocate two structures: The first for a pointer and size, the second for the results. For me, this sounds like an overkill, but maybe I'm missing something.

[Laczen]

It is more general and versatile, suppose that for some operation you would need 2 buffers each with their own size, this wouldn't fit in the pointer + size setup. As a bonus this also only needs 1 extra api function and not 2.

Maybe it would be better to use "context" (op_ctx) instead of code_param.

[Laczen]

I agree that the difference is small, but I don't see a reason to provide the *out. IMHO this is better replaced by a sizeof the struct that is passed to avoid casting errors/reading outside struct boundaries.

Size does not matter. The driver should only understand predefined structures as op context, there is no point to size them. If we provide size, how do we use it? Do we compare it against known sizes and drop unsupported? Does not seem to practical, it is rather one time error. Do we allow any size? What if user mismatches size between structures? Does giving size saves us from anything? We will just overwrite or read pass what we should, and this is in system mode, so we will not figure that out prior to returning to user mode anyway.

OK to drop the sizeof requirement,

Probably we would also need to define some "standard" struct layouts (in flash.h) to be passed to this function:

1. Structure for a simple command passed,

2. Structure for a read type command passed,

3. Structure for a write type command passed,

4. Structure for a read+write type command passed (with 2 buffers),

We already have API call for most of above cases so we know how they behave, and they are unique themselves and they already do not cover what we are trying to do, they cover what we had already done; the purpose of this API call, and the vendor range itself, is so that we could evaluate things we do not know yet by observing how users try to overcome problems in real scenarios. Then when we collect enough data, or what kind of problems users have how do they prefer to use API, how certain functionalities are provided by different vendors and then design common op code, for these functionalities, in system code space.

You might have overlooked the "type" that I inserted before "command ..." (and forgotten for the "simple" case 😀 ):
a. the get protection would be a "read type command structure",
b. the set protection would be a "write type command structure",

If these structures are not defined how would they be included by the user ? Would users have to include a vendor specific include file ? Where should these include files be placed ?

As illustrated by your examples I think all should be possible with the chosen function definition. I still don't see the benefit of the *out because a context needs to be provided anyhow and I don't like the obligation to provide NULL if *out is not needed. But you can decide.

[de-nordic]

You might have overlooked the "type" that I inserted before "command ..." (and forgotten for the "simple" case 😀 ): a. the get protection would be a "read type command structure", b. the set protection would be a "write type command structure",

I might have, I would be a really bad lawyer.

If these structures are not defined how would they be included by the user ? Would users have to include a vendor specific include file ? Where should these include files be placed ?

Yes. That is the idea #53905 (comment). When we standardize feature with system wide code, than user can switch to using it.
Non standard codes would not likely to be used in Zephyr code base, as they would start favoring some selected platform.
In case of the RD/WR locks on flash this is actually thing that user will know what they are doing and usage would be app specific, rather then something done in system; but when we finally have some common code and structures derived, after gaining experience with various platforms and mem chips, we could try to evaluate using this to set some app protections, by for example MCUboot, to prevent accidental writes over running application.

I am assemblysaurus from the argsonstack coding period, so passing unused params on stack before doing syscall/int is stuff I can still accept. I remember, though, one of my teachers mentioning to avoid designing more than five param functions to avoid unneeded stack ops, and because x86 didn't have that many registers back then.
Anyway gathering pros and cons here.

Pros/cons for the flash_ex_op(..., code, const in, out)

1. + No need for copy in in when only out pointer is needed;
2. + in can be used to pass integer, for simple operations that do not return anything (like set power level);
3. - Need to save-pass-restore register used by out and set it to NULL when not used;
4. + out can be used to either pass pointer to for object of predefined size, or pointer to structure that will define output and, being non-const, could be filled in in place by function;

Pros/cons for the flash_ex_op(..., in_out)

1. - No need to copy in in_out`` to get outpointer, unless we agree to makein_out` non-const, which I would rather not do (vs +on 1 in above list);
2. same as 2 in above;
3. + No need to save-pass-restore out register (vs - on 3 in above list, flash saved)
4. - if in_out is const and can not be written directly, just for being inconvenient, otherwise the in_out may point to some additional structure;

To explain more what I mean by 4:

struct something_in {
    uint32_t in_size;
    uint8_t *in_buf;
    uint32_t out_offset;  // Offset where to write not within out buffer;
};
struct something_out {
   uint32_t out_size;
   uint8_t *out_buf;
};
struct something_in sin = { .. some init };
struct something_out sout = { .out_size = max_out_buf_size, .out_buf = out_buf_ptr };
// When successful, the &sout will have .out_size replaced with size of data written to out_buf_ptr; the function can not directly modify the sin but can do that with sout;
ret = flash_ex_op(dev, code, &sin, &sout);
// Now we have function that does not use extra input info, and can still read/write the out.
ret = flash_ex_op(dev, code, NULL, &sout);

For me using separate in and out is more convenient, at the cost of passing NULL sometimes and one more set of store-pass-restore operations at call. It also allows to easily use the in (since the intptr_t) as basic integer parameter, for which you do not have to define extra input structure.

[Laczen]

I would still rather settle on flash_ex_op(struct device *device, uint32_t code, const void *code_ctx) because:

a. It is similar to disk io_ctl,
b. In the case a size passed for *out, there would be a deviation from first passing the pointer and then the length,
c. Even in the case a size is passed for *out probably another parameter e.g. a offset will need to be passed as well, so there would still be the need to setup a context,
d. The special use case where it is to be expected that only one value will be passed for power level will probably need to be standardized into a new (generic) api function,

[duda-patryk]

To make sure that I'm on the same page: by "copy in" and "copy out" you mean accordingly z_user_from_copy() and z_user_to_copy() used in a verifier as described in https://docs.zephyrproject.org/latest/kernel/usermode/syscalls.html#verification-memory-access-policies? Copy function requires size of structure it's going to copy, but this depends on the extended operation code, so the verifier needs to be implemented in the driver (similar to subsys/net/ip/net_if.c) instead of some dedicated file, is this correct?

The link above mentions that we need to make a copy to protect against situation in which userspace changes structure passed to kernel, after it was verified. Does it mean that we don't need to copy in structures that are const?

No need to copy in in_out to get out pointer, unless we agree to make in_out non-const, which I would rather not do (vs +on 1 in above list);

I'm not sure if I understand this correctly. In this case we actually need to copy in in order to get out pointer, don't we? Or we can merge input and output parameters in one structure (but it needs to be non-const).

It is similar to disk io_ctl,

flash_ex_op(struct device *device, uint32_t code, const void *code_ctx)
mmc_ioctl(struct sd_card *card, uint8_t cmd, void *buf)

The difference is that ioctl doesn't require the buffer to be const. This one keyword affects implementation, so IMO they are not similar.

[Laczen]

It seems that the syscall interface will require the standardization of the "context" passed to the flash_ex_op call. Regarding the difference with mmc_ioctl call, it is indeed different, but I don't think the mmc_ioctl call can be made in accordance with the syscall interface (regardless of wether it is a const or not it should pass a size).

I am thinking about a generic way that would allow passing the required info, but I haven't found one yet (even when splitting it up in a get and set method) taking into account that the parameter count should not be more than 4.

[Laczen]

@semihalf-duda-patryk, @de-nordic, in order to get this api in line with the syscall requirements the following should work:
a. The additional functions are split up in a get and set method,
b. The code parameter is converted into a struct io_code that consists of a code and a offset,

So the interface would look like:

struct io_code {
        uint32_t code;
        offset offset;
};

int flash_ex_op_set(const struct device *dev, const struct io_code code, const void *data, size_t len);
int flash_ex_op_get(const struct device *dev, const struct io_code code, void *data, size_t len);

This somewhat limits the possibilities, but even the most complicated function I can think of should be possible with a combination of a set+get command. And it keeps the number of function parameters limited to 4.

What is your opinion?

[duda-patryk]

b. The code parameter is converted into a struct io_code that consists of a code and a offset,

What is the purpose of offset in this case?

[Laczen]

In many cases the offset might not be needed.

An example I could think of: suppose you would need to set the protection at a certain address by providing a special code (only known to the user, not the driver), the offset could be used to provide the address, while the data is used to provide the code.

This could of course be combined into the data field, but this might require copying the data to some intermediate memory range such that syscall can be used to provide adequate protection.

[duda-patryk]

set the protection at a certain address by providing a special code (only known to the user, not the driver), the offset could be used to provide the address

It's sounds like offset can be used to write to flash-controller registers directly. I think this is not something we want to expose to user.

[Laczen]

set the protection at a certain address by providing a special code (only known to the user, not the driver), the offset could be used to provide the address

It's sounds like offset can be used to write to flash-controller registers directly. I think this is not something we want to expose to user.

Only when it is exposed by the driver. Regarding the special code I was thinking about security unlock data (e.g. a password or passphrase).

[duda-patryk]

Added z_vrfy_flash_ex_op(). In https://github.com/zephyrproject-rtos/zephyr/pull/52980/files#diff-81124c950e11733d9c2cf36ff0faf33265ef0d51c7753406e3f55c1cd024b41d you can see verification for vendor codes.

[Laczen]

Added z_vrfy_flash_ex_op(). In https://github.com/zephyrproject-rtos/zephyr/pull/52980/files#diff-81124c950e11733d9c2cf36ff0faf33265ef0d51c7753406e3f55c1cd024b41d you can see verification for vendor codes.

Yeah, I don't like this to much, pushing all vendor iocodes into flash_handlers.c to be able to provide adequate protection seems like a bad solution.

[duda-patryk]

Added z_vrfy_flash_ex_op(). In https://github.com/zephyrproject-rtos/zephyr/pull/52980/files#diff-81124c950e11733d9c2cf36ff0faf33265ef0d51c7753406e3f55c1cd024b41d you can see verification for vendor codes.

Yeah, I don't like this to much, pushing all vendor iocodes into flash_handlers.c to be able to provide adequate protection seems like a bad solution.

Yup. I agree that putting vendor codes to flash_handlers.c stinks. We can move it to some other file, but z_vrfy_flash_ex_op() will still need to call vendor verification function.

I wonder, what if we had a two different flash-controllers (e.g. one internal and one external)? We will need to check which vendor verification function we need to call.

@de-nordic what do you think about it?

[Laczen]

@semihalf-duda-patryk, if we could standardize the interface that allows syscall to do its functionality there would be no need for z_vrfy_flash_ex_op to call vendor functions.

[duda-patryk]

@semihalf-duda-patryk, if we could standardize the interface that allows syscall to do its functionality there would be no need for z_vrfy_flash_ex_op to call vendor functions.

We are trying to add a syscall which will be used for many purposes. Our interface will be common denominator of all functionalities we are trying to expose with it - some code to describe operation and buffer used by it.

https://docs.zephyrproject.org/latest/kernel/usermode/syscalls.html#verification-memory-access-policies mentions that:

If the kernel makes any logical decisions based on the contents of this memory, this can open
up the kernel to attacks even if checking is done. This is a class of exploits known as TOCTOU
(Time Of Check to Time Of Use).

The proper procedure to mitigate these attacks is to make a copies in the verification function,
and only perform parameter checks on the copies, which user threads will never have access to. 

It means that we have to make a copy of provided buffer in verification function. To do that without vendor specific structures definitions, we need to provide size of a buffer while calling syscall.

Another thing is that:

Many system calls pass in structures or even linked data structures. All should be copied.
Typically this is done by allocating copies on the stack.

If we want to avoid interpreting provided buffer in verification function we can't pass any pointers inside the structure.
I think, in case of additional data (e.g. if we want to implement some extended operation for writing,) we could use flexible array member (zero length array) at the end of structure, but it also requires passing size of the buffer to the syscall.

[duda-patryk]

@de-nordic Is there anything else I should fix?

[de-nordic]

Just add one more commit with release notes update, informing users on addition of the API call and Kconfigs.

[duda-patryk]

@de-nordic Sure. Do you have some example commit with release note update?

[de-nordic]

@de-nordic Sure. Do you have some example commit with release note update?

Something like 516b673 but in Flash section of drivers.
Probably something like "Flash API has now new API call flash_ex_op, which calls ex_op callback if provided by a flash driver,
that allows to perform extra operations, on flash devices, defined by Flash API and defined by vendor specific header files.
The ex_op member of :c:struct:flash_driver_api is enabled by :kconfig:option:BLAH, which is only available when at least one driver selects :kconfig:option:HAS_BLAH."

Btw. Catch me on discord under de-nordic, if you need to discuss this with lower latency.