gateway/uplink: bound writes against the block-pool constant

[fderepas]

Summary

pouch_gateway_uplink_write() (src/gateway/uplink.c) bounds its buf_write
into a block-pool element against GW_BLOCK_MAX_BYTES, which is defined as the
raw CONFIG_POUCH_BLOCK_SIZE. The pool element itself is dimensioned from
MAX_PLAINTEXT_BLOCK_SIZE, which uses CONFIG_POUCH_BLOCK_SIZE rounded down to
a power of two. For any non-power-of-two CONFIG_POUCH_BLOCK_SIZE, the write
bound exceeds the element capacity and peer-supplied bytes overflow the pool
element. This PR derives the bound from the same constant as the allocation and
adds a build-time power-of-two assertion.

Severity: Medium — peer-controlled pool overflow, config-conditional (requires a
non-power-of-two CONFIG_POUCH_BLOCK_SIZE; not reachable at the default 512).

The defect

src/gateway/uplink.c:

/*
 * Max bytes per gateway block.  Each pouch_buf slot holds at least
 * CONFIG_POUCH_BLOCK_SIZE bytes; we conservatively use that as the
 * per-block capacity.
 */
#define GW_BLOCK_MAX_BYTES CONFIG_POUCH_BLOCK_SIZE          /* raw Kconfig value */

int pouch_gateway_uplink_write(struct pouch_gateway_uplink *uplink,
                               const uint8_t *payload, size_t len)
{
    while (len)
    {
        if (uplink->wblock != NULL && buf_size_get(uplink->wblock) >= GW_BLOCK_MAX_BYTES)
            submit_wblock(uplink);
        if (uplink->wblock == NULL) {
            uplink->wblock = blockbuf_alloc(POUCH_NO_WAIT);   /* a POOL element */
            if (uplink->wblock == NULL) return -ENOMEM;
        }
        size_t space = GW_BLOCK_MAX_BYTES - buf_size_get(uplink->wblock);
        size_t bytes_to_copy = MIN(len, space);
        buf_write(uplink->wblock, payload, bytes_to_copy);    /* OOB when bound > capacity */
        len -= bytes_to_copy; payload += bytes_to_copy;
    }
    return 0;
}

This has the correct MIN-and-loop shape — it just bounds against the wrong
constant. The pool element is sized by MAX_PLAINTEXT_BLOCK_SIZE
(port/zephyr/blockbuf.c: K_MEM_SLAB_DEFINE(blockbuf, WB_UP(POUCH_BUF_OVERHEAD + MAX_PLAINTEXT_BLOCK_SIZE), …)), and that constant is rounded down (src/block.h):

#define MAX_BLOCK_PAYLOAD_SIZE_LOG LOG2(CONFIG_POUCH_BLOCK_SIZE)
#define MAX_BLOCK_PAYLOAD_SIZE     (1 << MAX_BLOCK_PAYLOAD_SIZE_LOG)
#define BLOCK_HEADER_SIZE          3
#define MAX_PLAINTEXT_BLOCK_SIZE   (BLOCK_HEADER_SIZE + MAX_BLOCK_PAYLOAD_SIZE)

with LOG2 a floor (port/include/pouch/port.h):

#define LOG2(x) (31 - __builtin_clz(x))     /* floor(log2 x) — rounds DOWN */

So the write bound is CONFIG_POUCH_BLOCK_SIZE, the capacity is
3 + 2^floor(log2(CONFIG_POUCH_BLOCK_SIZE)), and the comment's claim that the slot
"holds at least CONFIG_POUCH_BLOCK_SIZE bytes" is false for every non-power-of-two
value.

Arithmetic

CONFIG_POUCH_BLOCK_SIZE	capacity MAX_PLAINTEXT_BLOCK_SIZE	bound GW_BLOCK_MAX_BYTES	bound − capacity
512 (default)	515	512	−3 (safe)
1024	1027	1024	−3 (safe)
500	259	500	+241
600	515	600	+85
1000	515	1000	+485
1500	1027	1500	+473

At powers of two the bound is exactly 3 bytes under capacity (the block header), so
the code is safe by coincidence of the header size, not by construction.

The overflow needs no large single write: uplink->wblock persists across calls and
is only submitted at buf_size_get >= GW_BLOCK_MAX_BYTES, so ordinary small bearer
chunks accumulate. At CONFIG_POUCH_BLOCK_SIZE = 1000, five 128-byte BLE-sized
recv() chunks put 640 bytes into a 515-byte element.

Trust source / reachability

bearer recv (BLE GATT / node-facing transport)
  broker_endpoint_uplink.recv(bearer, payload, len)      src/transport/endpoints/broker/uplink.c
    pouch_gateway_uplink_write(node->uplink, payload, len)
      buf_write(uplink->wblock, payload, bytes_to_copy)   src/gateway/uplink.c

payload/len are the raw bytes a connected device pushed at the gateway; the
gateway forwards node uplinks to the cloud without decrypting them, so there is
no authentication gate on this path — the trust source is the peer device. Content
is fully peer-controlled; the overrun length is set by the configuration, not the
peer. On Zephyr the overrun lands in the next k_mem_slab element, whose first word
is the free-list next pointer (a write-what-where primitive). Reachability is
gated only on a non-power-of-two CONFIG_POUCH_BLOCK_SIZE, which the Kconfig accepts
with no range and no warning (see below).

Root-cause note

Because of the same rounding, a user who sets CONFIG_POUCH_BLOCK_SIZE = 1000
silently gets 512-byte blocks everywhere else (block_space_get,
MAX_CIPHERTEXT_BLOCK_SIZE, the slab element). One Kconfig option is consumed raw
in this one place and rounded in every other — that inconsistency, not the MIN
itself, is the bug. CONFIG_POUCH_BLOCK_SIZE (src/Kconfig) carries no range and
no power-of-two wording, and no BUILD_ASSERT in the tree constrains it (the one
power-of-two assert, port/zephyr/transport/coap/blockwise.c, constrains the
different CONFIG_POUCH_COAP_BLOCK_SIZE).

src/gateway/uplink.c is the only write bound in the tree spelled as a raw Kconfig
value rather than a block.h constant; the siblings get it right
(src/downlink.c bounds against MAX_CIPHERTEXT_BLOCK_SIZE, src/entry.c and
src/stream.c against block_space_get).

Reproduction (AddressSanitizer)

W6c-asan-test.c transcribes GW_BLOCK_MAX_BYTES, pouch_gateway_uplink_write,
buf_claim/buf_write, LOG2, the block.h macros, and the slab geometry
verbatim, and feeds 128-byte bearer chunks. The discriminator is the compile-time
CONFIG_POUCH_BLOCK_SIZE:

cc -g -fsanitize=address -DCONFIG_POUCH_BLOCK_SIZE=512  W6c-asan-test.c -o w6c && ./w6c   # safe
cc -g -fsanitize=address -DCONFIG_POUCH_BLOCK_SIZE=1000 W6c-asan-test.c -o w6c && ./w6c   # overflow

At 512 (power of two) it survives; at 1000 (non-power-of-two) ASan faults on the
fifth accumulated chunk:

CONFIG_POUCH_BLOCK_SIZE=1000 (pow2:NO) capacity(MAX_PLAINTEXT)=515 bound(GW_BLOCK_MAX_BYTES)=1000 exceeds_by=485
==…==ERROR: AddressSanitizer: heap-buffer-overflow … WRITE of size 128 …
    #1 … in buf_write                     W6c-asan-test.c:31
    #2 … in pouch_gateway_uplink_write    W6c-asan-test.c:45
… is located 0 bytes after 532-byte region      <-- the pool element, WB_UP(16 + 515)
SUMMARY: AddressSanitizer: heap-buffer-overflow … in buf_write

Rebuilding with -DFIX (bound = MAX_BLOCK_PAYLOAD_SIZE) at
CONFIG_POUCH_BLOCK_SIZE = 1000 survives cleanly — the same discriminating test the
raw witness describes.

Why the existing tests miss it

Two independent reasons: (1) tests/pouch/gateway/src/stub_blockbuf.c replaces the
slab with a 4096-byte malloc (8× the real element), the same double that masked
the gateway-downlink overflow; and (2) the tests only build at the default
power-of-two CONFIG_POUCH_BLOCK_SIZE, where the bug is unreachable by construction.
tests/pouch/gateway/src/uplink.c does push 20×200 = 4000 bytes — enough to overflow
a real 515-byte element several times — and passes only because of the stub.

The fix

Primary — derive the bound from the same constant as the allocation:

--- a/src/gateway/uplink.c
+++ b/src/gateway/uplink.c
@@
-/*
- * Max bytes per gateway block.  Each pouch_buf slot holds at least
- * CONFIG_POUCH_BLOCK_SIZE bytes; we conservatively use that as the
- * per-block capacity.
- */
-#define GW_BLOCK_MAX_BYTES CONFIG_POUCH_BLOCK_SIZE
+#include "../block.h"
+/*
+ * Max payload bytes per gateway block.  Bound writes against the SAME rounded
+ * constant the block pool is dimensioned from (MAX_PLAINTEXT_BLOCK_SIZE =
+ * BLOCK_HEADER_SIZE + MAX_BLOCK_PAYLOAD_SIZE), not the raw CONFIG_POUCH_BLOCK_SIZE,
+ * which LOG2() rounds down for the allocation but not here.
+ */
+#define GW_BLOCK_MAX_BYTES MAX_BLOCK_PAYLOAD_SIZE

MAX_BLOCK_PAYLOAD_SIZE (= MAX_PLAINTEXT_BLOCK_SIZE − BLOCK_HEADER_SIZE) is the
rounded payload the pool element is built for, so the bound now follows the
allocation by construction and equals the previous value at every power of two.

Secondary — fail the build on the surprising rounding (independently worth
having, since a user asking for 1000-byte blocks silently getting 512 is itself a
defect):

--- a/src/block.h
+++ b/src/block.h
@@
+POUCH_STATIC_ASSERT((CONFIG_POUCH_BLOCK_SIZE & (CONFIG_POUCH_BLOCK_SIZE - 1)) == 0,
+                    "CONFIG_POUCH_BLOCK_SIZE must be a power of two");

The secondary fix alone is not sufficient: asserting a side condition makes
this miscalculation impossible but leaves the invariant "write bound == allocation
size" resting on a coincidence between two unrelated files. Only the primary fix
makes the bound follow the allocation by construction. Recommend both.

How this was found

This was found by creating a model of Pouch using a theorem prover, then proving using
Weakest Preconditions the expected properties on the source code. WP cannot discharge
the buf_write valid_dest obligation for a non-power-of-two CONFIG_POUCH_BLOCK_SIZE
because the write bound (GW_BLOCK_MAX_BYTES) exceeds the proven element capacity
(MAX_PLAINTEXT_BLOCK_SIZE); the contrast with the sibling bounds that use block.h
constants pinpointed the raw-Kconfig bound. Confirmed independently under
AddressSanitizer (above), config-conditionally, with a faithful transcription of the
shipped uplink/block/slab geometry.

[hasheddan]

Great find @fderepas! I left some notes below regarding the new comments.

[hasheddan]

I think the original comment (with CONFIG_POUCH_BLOCK_SIZE replaced with MAX_BLOCK_PAYLOAD_SIZE) is more appropriate here. This is another example of a comment describing the change rather than the result, when the change is not visible to someone who is reading the code after the change is made. I think this new comment would make for an excellent commit message though.

[trond-snekvik]

I actually explicitly want to avoid this, as the rounding down is deliberate on the device side. As the block size always has to be a power of two, rounding down works well here as well, as long as we account for it when calculating the capacity of the block.

I made a counter proposal for this problem in #336, which changes the uplink module to use the appropriate block API for calculating the capacity of the buffer.