Skip to content

fix(incoming): treat GRANT offset as unsigned in homa_grant_pkt - #99

Open
randomizedcoder wants to merge 1 commit into
PlatformLab:mainfrom
randomizedcoder:fix/grant-offset-validation
Open

randomizedcoder wants to merge 1 commit into
PlatformLab:mainfrom
randomizedcoder:fix/grant-offset-validation

Conversation

@randomizedcoder

Copy link
Copy Markdown

Bug: GRANT offset read as signed in homa_grant_pkt()

struct homa_grant_hdr *h = (struct homa_grant_hdr *)skb->data;
int new_offset = ntohl(h->offset);
...
if (new_offset > rpc->msgout.granted) {
    rpc->msgout.granted = new_offset;
    if (new_offset > rpc->msgout.length)
        rpc->msgout.granted = rpc->msgout.length;   /* clamp */
}

ntohl() yields an unsigned 32-bit value, but new_offset is a signed int. An offset with the top bit set (≥ 2 GB) becomes negative, so new_offset > rpc->msgout.granted is false and the grant is silently dropped — the clamp-to-length path that was written to bound oversized grants never runs.

Fix

Read the wire offset as u32, and clamp before assigning into the signed rpc->msgout.granted (an int) so an oversized value never lands in the signed field even transiently:

-	int new_offset = ntohl(h->offset);
+	u32 new_offset = ntohl(h->offset);
 	...
-	if (new_offset > rpc->msgout.granted) {
-		rpc->msgout.granted = new_offset;
-		if (new_offset > rpc->msgout.length)
-			rpc->msgout.granted = rpc->msgout.length;
-	}
+	if (new_offset > rpc->msgout.granted)
+		rpc->msgout.granted = min_t(u32, new_offset,
+					    rpc->msgout.length);

The min_t(u32, …) result is always ≤ msgout.length, so the store into the signed field is well-defined, and grants remain capped at the message length. min_t(u32, …) matches existing idiom in homa_outgoing.c.

Verification (TDD, red → green)

Table-driven test homa_grant_pkt__offset_is_unsigned, on a 20000-byte outgoing message with granted pre-set to 5000:

row grant offset before fix after fix
normal_grant_advances 12000 granted 12000 granted 12000
grant_past_end_clamps_to_length 25000 granted 20000 granted 20000
high_bit_offset_clamps_not_ignored 0x80000000 granted 5000 (dropped) granted 20000 (clamped)
Red:   Expected exp_granted (20000) == crpc->msgout.granted (5000)   [high-bit row]
Green: homa_grant_pkt__offset_is_unsigned ... 1 / 1 passed
       homa_incoming (whole fixture) ......... 123 / 123 passed

The normal/past-end rows reproduce the existing homa_grant_pkt__basics (12000) and homa_grant_pkt__grant_past_end_of_message (25000→20000) behavior. Built and run against the test/ kselftest harness (ASan on).

Uses the table-driven test style proposed in the first PR of this series.

A GRANT packet's offset is an unsigned 32-bit wire field, but
homa_grant_pkt() reads it into a signed int:

    int new_offset = ntohl(h->offset);

An offset with the top bit set (>= 2GB) becomes negative, so
`new_offset > rpc->msgout.granted` is false and the grant is silently
dropped instead of being bounded. Read it as u32.

Clamp before the assignment, in unsigned space, so the oversized value
never lands in the signed rpc->msgout.granted (int) even transiently:

    rpc->msgout.granted = min_t(u32, new_offset, rpc->msgout.length);

The result is always <= msgout.length, so the store into the signed
field is well-defined, and grants are capped at the message length as
before.

Adds a table-driven test, homa_grant_pkt__offset_is_unsigned: the
high-bit row advances granted to the message length (20000) after the
fix, versus being ignored (granted stays 5000) before it. The normal and
past-end rows match the existing __basics / __grant_past_end_of_message
behavior; the incoming suite stays green (123/123).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant