userspace: Additional checks in K_SYSCALL_MEMORY

[ceolin]

This macros needed additional checks before invoking arch_buffer_validate.

• size can not be less then 0. Some functions invoke this macro using signed type which will be promote to unsigned when invoking arch_buffer_validate. We need to do an early check.
• We need to check for possible overflow, since a malicious user application could use a negative number that would be promoted to a big value that would cause a integer overflow when adding it to the buffer address, leading to invalid checks.

[henrikbrixandersen]

Hmm. So some of our APIs actually allow passing in a zero length (e.g. the EEPROM API) and this is tested (in test_zero_length_write, which now fails the test case due to these changes).

[ceolin]

Hmm. So some of our APIs actually allow passing in a zero length (e.g. the EEPROM API) and this is tested (in test_zero_length_write, which now fails the test case due to these changes).

the problem is that arch_buffer_validate is a low level API and AFAIU in some architectures size 0 is undefined behavior. Let me check these particular use cases.

[ceolin]

@henrikbrixandersen I end up adding a redundant check in these APIs since I think it is better to have that 0 length check in that macro to avoid any undefined behavior.

[Laczen]

@henrikbrixandersen I end up adding a redundant check in these APIs since I think it is better to have that 0 length check in that macro to avoid any undefined behavior.

There are many api's having the same problem with the length 0 check (flash, bbram, retained_mem, ...). It would be better to correct it at the arch_buffer_validate level.

[ceolin]

@henrikbrixandersen I end up adding a redundant check in these APIs since I think it is better to have that 0 length check in that macro to avoid any undefined behavior.

There are many api's having the same problem with the length 0 check (flash, bbram, retained_mem, ...). It would be better to correct it at the arch_buffer_validate level.

if we change arch_buffer_validate to return false we will end up with the same problem, ultimately what it means to try to read / write from a 0 length memory ?

I can remove the > 0 check there and continue how it is now, since the visible problem is mostly with huge or negative length that causes an overflow. But the fact that 0 length buffers can lead to undefined behavior in that function is really scary

[Laczen]

@henrikbrixandersen I end up adding a redundant check in these APIs since I think it is better to have that 0 length check in that macro to avoid any undefined behavior.

There are many api's having the same problem with the length 0 check (flash, bbram, retained_mem, ...). It would be better to correct it at the arch_buffer_validate level.

if we change arch_buffer_validate to return false we will end up with the same problem, ultimately what it means to try to read / write from a 0 length memory ?

Writing/reading with 0 length memory in all cases I know means: do nothing. A well written driver will not do anything in this case. In practice this will never be used. I don't understand why all drivers try to catch this early, this is just extra code that is never used in practice.

The arch_buffer_validate routine should accept len = 0 for any void *data. With data = NULL it should only accept len = 0.

[ceolin]

@henrikbrixandersen I end up adding a redundant check in these APIs since I think it is better to have that 0 length check in that macro to avoid any undefined behavior.

There are many api's having the same problem with the length 0 check (flash, bbram, retained_mem, ...). It would be better to correct it at the arch_buffer_validate level.

if we change arch_buffer_validate to return false we will end up with the same problem, ultimately what it means to try to read / write from a 0 length memory ?

Writing/reading with 0 length memory in all cases I know means: do nothing. A well written driver will not do anything in this case. In practice this will never be used. I don't understand why all drivers try to catch this early, this is just extra code that is never used in practice.

The arch_buffer_validate routine should accept len = 0 for any void *data. With data = NULL it should only accept len = 0.

I have dropped the check "> 0" since there are APIs relying on it. Instead I changed ">= 0" that fix the real problem because some functions invoke this macro using signed type which will be promote to unsigned when invoking arch_buffer_validate.

[Laczen]

@ceolin This change seems to have triggered many Coverity CID issues.

[finikorg]

Same for me, size is usually unsigned.