net: lwm2m: Fix issues reported by undefined behavior sanitizer #92317
Conversation
Check if value pointer is not NULL before passing it to memcmp() inside lwm2m_engine_set(). As the function actually expects that the value pointer can be NULL in case resource value is cleared (there is a test case for such behavior), validate that len value is actually 0 if NULL value is provided, to avoid unexpected behavior in other parts of the function. Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
| temp = temp * 10ULL + (c - '0'); | ||
| if (temp > (INT64_MAX + (neg ? 1ULL : 0ULL))) { | ||
| return -EINVAL; | ||
| } |
There was a problem hiding this comment.
| temp = temp * 10ULL + (c - '0'); | |
| if (temp > (INT64_MAX + (neg ? 1ULL : 0ULL))) { | |
| return -EINVAL; | |
| } | |
| if (temp > (INT64_MAX/10)) { | |
| return -EINVAL; | |
| } | |
| temp = temp * 10ULL + (c - '0'); |
I think the temp might roll-over before the if-check, so maybe it needs to be checked beforehand.
There was a problem hiding this comment.
Good point, I've updated the check for json and plain_text.
There was a problem hiding this comment.
On the second though, the original proposal was fine, the roll-over is not a concern as we update uint64_t variable and then compare against int64_t limits.
rlubos
force-pushed
the
net/lwm2m-fix-ubsan-warning
branch
from
1e93bc9 to
1ed0df0
Compare
|
Added fix for memory corruptions in |
subsys/net/lib/lwm2m/lwm2m_rw_json.c
Outdated
| if (temp > INT64_MAX / 10ULL) { | ||
| return -EINVAL; | ||
| } | ||
|
|
||
| temp = temp * 10ULL + (c - '0'); |
There was a problem hiding this comment.
INT64_MAX = 9223372036854775807 so 922337203685477580 once divided by 10, hence you're basically allowing 9223372036854775808 and 9223372036854775809 as valid values (confirmed by testing)
also make sure that when you fix it you still allow -9223372036854775808 (INT64_MIN) to be a valid input. Currently if the highlighted code were to work as I think you wanted it to, it would have not allowed for temp to end up being 9223372036854775808
There was a problem hiding this comment.
Might be a good idea to also add some tests, I guess :)
There was a problem hiding this comment.
Good catch, on the second thought my original proposal (#92317 (comment)) was fine, the overflow is not a concern as temp is uint64_t and we compare against int64_t limits. I'll add a test for the overflow case too.
Verify if the integer value being parsed does not overflow int64_t type and report an error in such cases. Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
Verify if the integer value being parsed does not overflow int64_t type and report an error in such cases. Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
* Simplify the logic of the get_number() function to address the corner cases reported by UBSAN regarding byte-swapping signed integer values. The existing logic was actually only valid for little-endian systems, as it expected that the bytes written from the packet buffer will be stored at the beginning of the int64_t memory, plus the actual byte-swapping with signed integer casts inside was hard to follow. Switch to a plain uint8_t buffer for integer readout, and use dedicated system function to convert the big-endian data in the buffer into unsigned integer in the system endianness, followed by the final cast to a signed value. * Add explicit cast to uint32_t in put_objlnk() to prevent warning about not-fitting integer after byte shift, and update the result type to uint32_t as well. * Switch to buffer with sys_put_be16/32/64 when writing integers due to similar warnings about byte-swapping signed values. Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
Check if there actually is data to copy before calling memcpy() to prevent potentially calling memcpy() with NULL value pointer. Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
LwM2M client context was defined on stack in the test function, however it could still be in use when the test ended, as the actual LwM2M teardown took place in a common "after" test function. In result, the Lwm2M context content could be corrupted. Additionally, increase the system work queue stack size, as the stack overflowed. Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
Add checks for int64_t overflows when parsing. Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
Add checks for int64_t overflows when parsing. Signed-off-by: Robert Lubos <robert.lubos@nordicsemi.no>
rlubos
force-pushed
the
net/lwm2m-fix-ubsan-warning
branch
from
a11a335 to
67e65bf
Compare
|
Updated the int64_t overflow check + added some extra test coverage for this. |
|
fabiobaltieri
merged commit 7b73998
into
zephyrproject-rtos:main
5 participants
For details, please check individual commits.