syscalls: arm: Fix possible overflow in is_in_region function #23239

ceolin · 2020-03-03T22:42:50Z

This function is widely used by functions that validate memory
buffers. Macros used to check permissions, like Z_SYSCALL_MEMORY_READ
and Z_SYSCALL_MEMORY_WRITE, use these functions to check that a
pointers passed by user threads in a syscall.

Signed-off-by: Flavio Ceolin [email protected]

arch/arm/core/aarch32/cortex_m/mpu/arm_mpu_v7_internal.h

jhedberg · 2020-03-04T10:46:50Z

@ceolin I've restarted Shippable a few times, but still keeps failing - is it because of something in this PR?

jhedberg · 2020-03-04T11:00:01Z

@ceolin I've restarted Shippable a few times, but still keeps failing - is it because of something in this PR?

Answering myself: seems like it is:

INFO    - /home/buildslave/src/github.com/zephyrproject-rtos/zephyr/sanity-out/mps2_an385/tests/kernel/pipe/pipe/kernel.pipe/handler.log
ERROR   - *** Booting Zephyr OS build v2.2.0-rc3-25-g426ddd95ee73  ***
Running test suite test_pipe
===================================================================
starting test - test_pipe_on_single_elements
E: syscall z_vrfy_k_pipe_put failed check: Memory region 0x200001c1 (size 0) read access denied
E: r0/a1:  0x00000000  r1/a2:  0x00000000  r2/a3:  0x00000000
E: r3/a4:  0x00000000 r12/ip:  0x00000000 r14/lr:  0x00000000
E:  xpsr:  0x00000000
E: Faulting instruction address (r15/pc): 0x00000000
E: >>> ZEPHYR FATAL ERROR 3: Kernel oops on CPU 0
E: Current thread: 0x200005d8 (unknown)
Caught system error -- reason 3

ceolin · 2020-03-04T16:17:01Z

@jhedberg there are two issues that we need to figure out how to handle

There is one pipe tests that pass a 0 data buffer. Is it valid ? 0 data buffer was causing and underflow and magically going to implementation function. I can put a new test in the verification function to return 0 in that situation and not going to check the memory buffer
poll test is failing for the same problem, one test with 0 events. Same thing the issue above, but here is more confuse because of the expected behavior, currently with 0 elements it returns EAGAIN when timeout expires, what is not possible to do in the verification function. I could return -EINVAL (that looks the right thing), but then the test will look really weird, because when the test executes from supervisor thread it will return -EAGAIN and when it runs from user thread will return -EINVAL.

any thoughts ?

arch/arm/core/aarch32/cortex_m/mpu/arm_mpu_v7_internal.h

ioannisg · 2020-03-04T16:34:24Z

arch/arm/core/aarch32/cortex_m/mpu/arm_mpu_v7_internal.h

+		return 0;
+	}
+
+	if (start >= r_addr_start && end <= r_addr_end) {


and i the nxp_mpu.c

ceolin · 2020-03-04T18:51:26Z

@jhedberg there are two issues that we need to figure out how to handle

There is one pipe tests that pass a 0 data buffer. Is it valid ? 0 data buffer was causing and underflow and magically going to implementation function. I can put a new test in the verification function to return 0 in that situation and not going to check the memory buffer

poll test is failing for the same problem, one test with 0 events. Same thing the issue above, but here is more confuse because of the expected behavior, currently with 0 elements it returns EAGAIN when timeout expires, what is not possible to do in the verification function. I could return -EINVAL (that looks the right thing), but then the test will look really weird, because when the test executes from supervisor thread it will return -EAGAIN and when it runs from user thread will return -EINVAL.

any thoughts ?

@andyross @andrewboie ideas ?

ioannisg · 2020-03-05T07:46:20Z

arch/arm/core/aarch32/cortex_m/mpu/arm_mpu_v7_internal.h

@@ -216,7 +217,11 @@ static inline int is_in_region(u32_t r_index, u32_t start, u32_t size)
 			MPU_RASR_SIZE_Pos) + 1;
 	r_addr_end = r_addr_start + (1UL << r_size_lshift) - 1;

-	if (start >= r_addr_start && (start + size - 1) <= r_addr_end) {
+	if (size == 0 || __builtin_add_overflow(start, size - 1, &end)) {


Also, if you want the function to return false when size is zero, I believe this needs to be documented.
Furthermore, I think the case of zero "size", should be better checked at the caller function, mpu_buffer_validate, or even documented as a non-allowed input in the common arch interface API, arch_buffer_validate. @andrewboie what do you think?

yeah, that is weird, but I believe x86 works same way, I'll change the pr to allows size 0, what means just check whether or not the buffer address is in the given region. This will fix the problem and keeps the current behavior

whether or not allow size 0 can be addressed / discussed later to no block the release, since it seems not to be a bug. The questions regarding poll and pipe behavior are still open IMHO.

This function is widely used by functions that validate memory buffers. Macros used to check permissions, like Z_SYSCALL_MEMORY_READ and Z_SYSCALL_MEMORY_WRITE, use these functions to check that a pointers passed by user threads in a syscall. Signed-off-by: Flavio Ceolin <[email protected]>

jhedberg · 2020-03-05T22:27:05Z

@ioannisg @stephanosio are you guys ok with the latest version of this PR? I'd like some more reviews than just my own before merging.

ioannisg · 2020-03-05T22:27:50Z

@ioannisg @stephanosio are you guys ok with the latest version of this PR? I'd like some more reviews than just my own before merging.

Will check this again early tomorrow morning @jhedberg

jhedberg · 2020-03-06T12:18:14Z

Will check this again early tomorrow morning @jhedberg

@ioannisg any update on this?

ioannisg

@ceolin correct me if I am wrong, but now, it seems that supplying a zero sized buffer will result in the function returning true.This is a change compared to the original behavior, so I wonder if we need that.
could this PR just fix the overflow issue?

ceolin · 2020-03-06T17:20:00Z

@ceolin correct me if I am wrong, but now, it seems that supplying a zero sized buffer will result in the function returning true.This is a change compared to the original behavior, so I wonder if we need that.
could this PR just fix the overflow issue?

We were wrongly accepting size 0.
if (start >= r_addr_start && (start + size - 1) <= r_addr_end)
as you can see, the second condition will always be true for size == 0

Now, at least we are not comparing an address out of the "valid" range. My initial patch was not accepting size == 0 but then it broke some tests and the behavior was different from X86. My suggestion is merge it and fix the problems and then we can decide whether or not accept 0 size buffers, this will apply to X86 and ARC as well.

andrewboie · 2020-03-06T17:44:56Z

it depends on syscall, poll for example seems to return when the timeout expires, without anything valid, pipe just return 0

I understand the behavior, but not the semantics. Why is the test passing a 0 buffer in the first place? What is it testing?

ceolin · 2020-03-06T17:47:32Z

it depends on syscall, poll for example seems to return when the timeout expires, without anything valid, pipe just return 0

I understand the behavior, but not the semantics. Why is the test passing a 0 buffer in the first place? What is it testing?

The test is testing this corner case what is valid. It is a boundary that has to be tested, the problem is if we reject 0 size buffer at memory access level we trigger an exception what the tests and APIs don't expect

andrewboie · 2020-03-06T17:51:54Z

The test is testing this corner case what is valid. It is a boundary that has to be tested, the problem is if we reject 0 size buffer at memory access level we trigger an exception what the tests and APIs don't expect

So is a 0 size buffer valid or not?

andrewboie · 2020-03-06T17:52:13Z

And what does it mean to call these APIs with a 0 size buffer?

ceolin · 2020-03-06T18:17:05Z

The test is testing this corner case what is valid. It is a boundary that has to be tested, the problem is if we reject 0 size buffer at memory access level we trigger an exception what the tests and APIs don't expect

So is a 0 size buffer valid or not?

That is the trick part, currently it is, but not explicitly. Regarding the second question, the meaning is per basis API. Take a look in k_pipe_put

/**
 * @brief Write data to a pipe.
 *
 * This routine writes up to @a bytes_to_write bytes of data to @a pipe.
 *
 * @param pipe Address of the pipe.
 * @param data Address of data to write.
 * @param bytes_to_write Size of data (in bytes).
 * @param bytes_written Address of area to hold the number of bytes written.
 * @param min_xfer Minimum number of bytes to write.
 * @param timeout Non-negative waiting period to wait for the data to be written
 *                (in milliseconds), or one of the special values K_NO_WAIT
 *                and K_FOREVER.
 *
 * @retval 0 At least @a min_xfer bytes of data were written.
 * @retval -EIO Returned without waiting; zero data bytes were written.
 * @retval -EAGAIN Waiting period timed out; between zero and @a min_xfer
 *                 minus one data bytes were written.
 * @req K-PIPE-002
 */

According with it, if bytes_to_write and min_xfer are 0 this function should return 0, what is valid. The test is testing it. This one is easy because we can't test for this case before testing the memory access, but poll is more complicate, because it accepts 0 events and just return when the timeout is reached.

andrewboie · 2020-03-06T18:25:27Z

I see. Maybe for footprint/performance reasons we should leave (and document in the header) that arch_buffer_validate() on a 0-sized region is undefined behavior, and for these particular syscalls add explicit checks. What do you think? We can contemplate this for another PR. Regardless of what we decide, the API docs should be updated.

ceolin · 2020-03-06T19:20:01Z

I see. Maybe for footprint/performance reasons we should leave (and document in the header) that arch_buffer_validate() on a 0-sized region is undefined behavior, and for these particular syscalls add explicit checks. What do you think? We can contemplate this for another PR. Regardless of what we decide, the API docs should be updated.

Agree. Further we can have the initial patch that invalidates 0 size buffers and properly fix APIs.

Do you want me to update this commit with that info ?

ioannisg · 2020-03-06T22:14:47Z

@andrewboie @ceolin I would like us to update the API and agree on the behavior when the passed size is zero before merging this PR, since this is an essential part of the patch. Ideally, I would like the test-cases and the doc-fix to come within this PR :)

ioannisg

Please, submit also the fix for API documentation, as well as a test that covers this corner case of zero size.

ceolin · 2020-03-06T22:21:18Z

Please, submit also the fix for API documentation, as well as a test that covers this corner case of zero size.

doing it.

ceolin · 2020-03-06T22:45:02Z

Please, submit also the fix for API documentation, as well as a test that covers this corner case of zero size.

what exactly you want here ? I'll document that 0 size buffer is undefined behavior, we should not have test for it. That's said, pipe and poll tests are exercising this case, my opinion is that we fix these APIs (maybe others) later.

Documenting that 0 size buffer has undefined behavior. See: zephyrproject-rtos#23239 Signed-off-by: Flavio Ceolin <[email protected]>

andrewboie · 2020-03-07T05:46:08Z

It's not like this UB represents a security concern unless I'm missing something. Should not block this PR.

ioannisg · 2020-03-07T10:47:01Z

include/sys/arch_interface.h

@@ -578,6 +578,8 @@ void arch_mem_domain_destroy(struct k_mem_domain *domain);
 * if the supplied memory buffer spans multiple enabled memory management
 * regions (even if all such regions permit user access).
 *
+ * @warning 0 size buffer has undefined behavior.


That's exactly what I wanted :)

ioannisg · 2020-03-07T11:10:05Z

That's said, pipe and poll tests are exercising this case, my opinion is that we fix these APIs (maybe others) later.

Since we are now documenting that arch_buffer_validate cannot get a 0 size buffer, we need to open an issue for fixing the APIs related to the tests mentioned above

Documenting that 0 size buffer has undefined behavior. See: zephyrproject-rtos#23239 Signed-off-by: Flavio Ceolin <[email protected]>

Documenting that 0 size buffer has undefined behavior. See: #23239 Signed-off-by: Flavio Ceolin <[email protected]>

ceolin requested review from d3zd3z, jhedberg, ioannisg and andrewboie March 3, 2020 22:42

ceolin requested review from galak and MaureenHelm as code owners March 3, 2020 22:42

ceolin added the priority: high High impact/importance bug label Mar 3, 2020

ceolin added this to the v2.2.0 milestone Mar 3, 2020

ceolin added the area: Security Security label Mar 3, 2020

zephyrbot added the area: ARM ARM (32-bit) Architecture label Mar 3, 2020

stephanosio reviewed Mar 3, 2020

View reviewed changes

arch/arm/core/aarch32/cortex_m/mpu/arm_mpu_v7_internal.h Outdated Show resolved Hide resolved

ioannisg reviewed Mar 4, 2020

View reviewed changes

arch/arm/core/aarch32/cortex_m/mpu/arm_mpu_v7_internal.h Outdated Show resolved Hide resolved

ioannisg reviewed Mar 4, 2020

View reviewed changes

ceolin changed the title ~~wsyscalls: arm: Fix possible overflow in is_in_region function~~ syscalls: arm: Fix possible overflow in is_in_region function Mar 4, 2020

ceolin requested review from andyross and andrewboie and removed request for andrewboie March 4, 2020 23:43

ioannisg reviewed Mar 5, 2020

View reviewed changes

ceolin force-pushed the syscalls branch from 94a713a to c892fd3 Compare March 5, 2020 15:33

jhedberg approved these changes Mar 5, 2020

View reviewed changes

ioannisg reviewed Mar 6, 2020

View reviewed changes

sys: usermode: Document 0 size buffer memory check

eb77c7c

Documenting that 0 size buffer has undefined behavior. See: zephyrproject-rtos#23239 Signed-off-by: Flavio Ceolin <[email protected]>

ceolin requested review from carlescufi and nashif as code owners March 6, 2020 22:46

zephyrbot added the area: API Changes to public APIs label Mar 6, 2020

ioannisg reviewed Mar 7, 2020

View reviewed changes

ioannisg approved these changes Mar 7, 2020

View reviewed changes

jhedberg merged commit 4b2fc25 into zephyrproject-rtos:master Mar 7, 2020

ioannisg mentioned this pull request Mar 7, 2020

ARM: Core Stack Improvements/Bug fixes for 2.2 release #21201

Closed

18 tasks

hakehuang pushed a commit to hakehuang/zephyr that referenced this pull request Mar 18, 2020

sys: usermode: Document 0 size buffer memory check

2ee7f5d

Documenting that 0 size buffer has undefined behavior. See: zephyrproject-rtos#23239 Signed-off-by: Flavio Ceolin <[email protected]>

ceolin mentioned this pull request Mar 20, 2020

Backport v1.14: syscalls: arm: Fix possible overflow in is_in_region function #23653

Merged

ceolin pushed a commit to ceolin/zephyr that referenced this pull request Mar 20, 2020

sys: usermode: Document 0 size buffer memory check

e7568ad

Documenting that 0 size buffer has undefined behavior. See: zephyrproject-rtos#23239 Signed-off-by: Flavio Ceolin <[email protected]>

ceolin mentioned this pull request Mar 20, 2020

Backport v2.1: syscalls: arm: Fix possible overflow in is_in_region function #23654

Merged

ceolin pushed a commit to ceolin/zephyr that referenced this pull request Mar 24, 2020

sys: usermode: Document 0 size buffer memory check

edf2f0b

Documenting that 0 size buffer has undefined behavior. See: zephyrproject-rtos#23239 Signed-off-by: Flavio Ceolin <[email protected]>

nashif pushed a commit that referenced this pull request Mar 24, 2020

sys: usermode: Document 0 size buffer memory check

f032de6

Documenting that 0 size buffer has undefined behavior. See: #23239 Signed-off-by: Flavio Ceolin <[email protected]>

dleach02 pushed a commit that referenced this pull request May 5, 2020

sys: usermode: Document 0 size buffer memory check

2649e4c

Documenting that 0 size buffer has undefined behavior. See: #23239 Signed-off-by: Flavio Ceolin <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

syscalls: arm: Fix possible overflow in is_in_region function #23239

syscalls: arm: Fix possible overflow in is_in_region function #23239

ceolin commented Mar 3, 2020

jhedberg commented Mar 4, 2020

jhedberg commented Mar 4, 2020

ceolin commented Mar 4, 2020 •

edited

Loading

ioannisg Mar 4, 2020

ioannisg Mar 4, 2020

ceolin commented Mar 4, 2020

ioannisg Mar 5, 2020 •

edited

Loading

ceolin Mar 5, 2020

ceolin Mar 5, 2020

jhedberg commented Mar 5, 2020

ioannisg commented Mar 5, 2020

jhedberg commented Mar 6, 2020

ioannisg left a comment

ceolin commented Mar 6, 2020 •

edited

Loading

andrewboie commented Mar 6, 2020

ceolin commented Mar 6, 2020

andrewboie commented Mar 6, 2020

andrewboie commented Mar 6, 2020

ceolin commented Mar 6, 2020 •

edited

Loading

andrewboie commented Mar 6, 2020

ceolin commented Mar 6, 2020

ioannisg commented Mar 6, 2020

ioannisg left a comment

ceolin commented Mar 6, 2020

ceolin commented Mar 6, 2020

andrewboie commented Mar 7, 2020

ioannisg Mar 7, 2020

ioannisg commented Mar 7, 2020

syscalls: arm: Fix possible overflow in is_in_region function #23239

syscalls: arm: Fix possible overflow in is_in_region function #23239

Conversation

ceolin commented Mar 3, 2020

jhedberg commented Mar 4, 2020

jhedberg commented Mar 4, 2020

ceolin commented Mar 4, 2020 • edited Loading

ioannisg Mar 4, 2020

Choose a reason for hiding this comment

ioannisg Mar 4, 2020

Choose a reason for hiding this comment

ceolin commented Mar 4, 2020

ioannisg Mar 5, 2020 • edited Loading

Choose a reason for hiding this comment

ceolin Mar 5, 2020

Choose a reason for hiding this comment

ceolin Mar 5, 2020

Choose a reason for hiding this comment

jhedberg commented Mar 5, 2020

ioannisg commented Mar 5, 2020

jhedberg commented Mar 6, 2020

ioannisg left a comment

Choose a reason for hiding this comment

ceolin commented Mar 6, 2020 • edited Loading

andrewboie commented Mar 6, 2020

ceolin commented Mar 6, 2020

andrewboie commented Mar 6, 2020

andrewboie commented Mar 6, 2020

ceolin commented Mar 6, 2020 • edited Loading

andrewboie commented Mar 6, 2020

ceolin commented Mar 6, 2020

ioannisg commented Mar 6, 2020

ioannisg left a comment

Choose a reason for hiding this comment

ceolin commented Mar 6, 2020

ceolin commented Mar 6, 2020

andrewboie commented Mar 7, 2020

ioannisg Mar 7, 2020

Choose a reason for hiding this comment

ioannisg commented Mar 7, 2020

ceolin commented Mar 4, 2020 •

edited

Loading

ioannisg Mar 5, 2020 •

edited

Loading

ceolin commented Mar 6, 2020 •

edited

Loading

ceolin commented Mar 6, 2020 •

edited

Loading