mlx5: Fix unsigned underflow in dr_ste_copy_param() by ez-lbz · Pull Request #1813 · linux-rdma/rdma-core

ez-lbz · 2026-10-04T14:13:01Z

dr_ste_copy_param() guards its memcpy() with an upper bound only. Six
regions are parsed, and each one computes mask_sz - param_location without
establishing that mask_sz reaches param_location. When the caller's mask is
shorter than the fixed match header the subtraction wraps, and memcpy() is
asked for that many bytes into a 448 byte stack buffer. The rule that supplies
the mask is checked for an upper bound and alignment in dr_rule.c, never for a
lower one.

A mask that does not reach the region being parsed has no bytes to copy, so the
change guards the copy on there being something to copy rather than clamping the
length afterwards. All six sites are covered, and the change is local to
providers/mlx5/dr_ste.c.

Reproduction without mlx5 hardware: call dr_ste_copy_param() with a mask one
block shorter than param_location. AddressSanitizer reports
negative-size-param, and a build with _FORTIFY_SOURCE aborts in the same
place.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

alexvesker · 2026-10-11T15:29:39Z

-			       mask_sz - param_location);
+			if (mask_sz > param_location)
+				memcpy(tail_param, data + param_location,
+				       mask_sz - param_location);


This is a case in which a user passed bad input,
the match_sz and match_criteria do not match - this will cause an underflow,
the problem is that there can be only one tail_param
and if it is dirty from a previous operation, you will pass it to
dr_ste_copy_mask_spec
causing an incorrect field copy.

I need to go over the code and see if we had other protections in this area that I am missing,
but the fix is partial.

Confirmed and fixed. The shared buffer is now cleared before each partial copy, so the absent part of a section reads as zero rather than as the previous section's bytes. Verified over every section bit and every 4-byte mask_sz from 4 to 448: a short mask now yields a byte-identical result to the same mask zero-padded to the full header size.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

ez-lbz · 2026-10-12T03:19:04Z

You are right, and thanks for catching it -- the stale-buffer half is a real defect that my
first version did not address.

I reproduced it against the compiled function. With mask_sz = 100 and
OUTER|MISC|INNER|MISC2, the caller supplies 0xA0 for the outer section and 0xB0 for the
first 36 bytes of MISC, and nothing for INNER or MISC2. The result was:

outer.smac_47_16             = 0xa0a0a0a0
inner.smac_47_16             = 0xb0b0b0b0   <- caller sent no INNER bytes
misc2.outer_first_mpls_label = 0x000b0b0b

so the caller's MISC bytes were being copied into the INNER and MISC2 fields, exactly the
"incorrect field copy" you described.

It is worth noting why the two halves go together: on the unpatched code the underflow is
what stops execution before any later section reads the buffer. Once the copy is skipped
instead of underflowing, the later sections are reached and the leftovers become observable.
So fixing only the underflow would have traded a crash for silent corruption.

The change now clears the shared buffer before each partial copy, so the absent part reads as
zero, which is what the = {} initialiser and the clear argument already intend. I checked
the property exhaustively -- over all section bits and every 4-byte mask_sz from 4 to 448, a
short mask now produces a byte-identical dr_match_param to the same mask zero-padded to
DEVX_ST_SZ_BYTES(dr_match_param) (14224 combinations, 0 mismatches).

dr_matcher_copy_mask() always passes a full-size zeroed buffer, so it takes the direct path
and is unchanged; I re-checked the clear = true result byte for byte against the old code.

alexvesker · 2026-10-12T06:56:41Z

 		       uint64_t *mask_buf, size_t mask_sz, bool clear)
 {
 	uint8_t *data = (uint8_t *)mask_buf;
+	/* A section whose bytes are only partly present in the caller's mask is


I think the code explains it, please drop.

alexvesker · 2026-10-12T07:44:50Z

+	 * parsed from this buffer, so the absent part has to read as zero.  All
+	 * sections share it, hence the clear before each partial copy.
+	 */
 	char tail_param[DEVX_ST_SZ_BYTES(dr_match_param)] = {};


now we can drop the inital init "{}"

Dropped as well -- every use of the buffer now clears it first, so the initialiser was redundant.

alexvesker · 2026-10-12T08:00:12Z

 		    DEVX_ST_SZ_BYTES(dr_match_set_misc2)) {
-			memcpy(tail_param, data + param_location,
-			       mask_sz - param_location);
+			memset(tail_param, 0, sizeof(tail_param));


what you did solved the bug.
I think an easier to read logic will be:
this way we don't copy unless we are in range, no point in running with empty buffer (as intended before)

/* Tmp comment for you only: don't do copy_mask if nothing to copy */ if ((match_criteria & DR_MATCHER_CRITERIA_INNER) && (mask_sz > param_location)) { if (mask_sz >= param_location + DEVX_ST_SZ_BYTES(dr_match_spec)) { /* Tmp comment for you only: after the range or exactly in the range - likely() */ buff = data + param_location; } else { /* Tmp comment for you only: in middle of the range */ memset(tail_param, 0, sizeof(tail_param)); memcpy(tail_param, data + param_location, mask_sz - param_location); buff = tail_param; } /* Tmp comment for you only: will happen only if needed */ dr_ste_copy_mask_spec(buff, &set_param->inner, clear); }

Sorry for the delay, and thanks for the nudge -- done now. I took your structure for all seven sections: a section is parsed only when the mask reaches into it, the direct pointer is used when the mask covers the whole section, and otherwise the scratch buffer is cleared and filled with what is present. Also dropped the comment and the "{}" initialiser as you asked. Details in the review replies.

dr_ste_copy_param() parses the caller's match mask one section at a time. A section that the mask does not reach was parsed from a single shared scratch buffer: char tail_param[DEVX_ST_SZ_BYTES(dr_match_param)] = {}; ... if (match_criteria & DR_MATCHER_CRITERIA_MISC) { if (mask_sz < param_location + DEVX_ST_SZ_BYTES(dr_match_set_misc)) { memcpy(tail_param, data + param_location, mask_sz - param_location); buff = tail_param; } else { buff = data + param_location; } dr_ste_copy_mask_misc(buff, &set_param->misc, clear); } Two defects follow from that. First, the copy length is computed without establishing that the mask reaches the section. When mask_sz is smaller than param_location the subtraction is done in size_t and underflows, so memcpy() is asked for about SIZE_MAX bytes: it reads far past the caller's buffer and writes past the 448 byte scratch buffer. The rule path checks the mask for an upper bound and alignment only (dr_rule.c), never for a lower one. Second, the buffer is shared by all seven sections and was initialised only once at function entry. A section whose mask_sz falls inside it copies the bytes that are present and leaves the remainder holding what the previous section left behind. Those bytes are then read as if the caller had supplied them, so fields the caller never specified take values from a different section of its own mask. The initialiser shows the intent is for the absent part to read as zero. A section is now parsed only when the mask reaches into it, and the scratch buffer is cleared before each partial copy, so the absent part reads as zero: if ((match_criteria & DR_MATCHER_CRITERIA_MISC) && (mask_sz > param_location)) { if (mask_sz >= param_location + DEVX_ST_SZ_BYTES(dr_match_set_misc)) { buff = data + param_location; } else { memset(tail_param, 0, sizeof(tail_param)); memcpy(tail_param, data + param_location, mask_sz - param_location); buff = tail_param; } dr_ste_copy_mask_misc(buff, &set_param->misc, clear); } A mask shorter than the match header then behaves exactly as the same mask zero padded to DEVX_ST_SZ_BYTES(dr_match_param). set_param is zeroed by the caller that can reach the partial path, and the other caller, dr_matcher_copy_mask(), always passes a full size buffer, so it takes the direct path and is unaffected. Fixes: 7ec30b8 ("mlx5: Add Steering entry (STE) utilities") Signed-off-by: Li Bingzhuo <lbz2770828522@gmail.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

ez-lbz · 2026-10-15T08:15:47Z

Both points applied, and I took the structure you sketched for all seven sections rather than only
the one you quoted. A section is now parsed only when the mask reaches into it, the caller's
pointer is used when the mask covers the whole section, and otherwise the scratch buffer is
cleared and filled with the bytes that are present:

if ((match_criteria & DR_MATCHER_CRITERIA_MISC) &&
    (mask_sz > param_location)) {
        if (mask_sz >=
            param_location + DEVX_ST_SZ_BYTES(dr_match_set_misc)) {
                buff = data + param_location;
        } else {
                memset(tail_param, 0, sizeof(tail_param));
                memcpy(tail_param, data + param_location,
                       mask_sz - param_location);
                buff = tail_param;
        }
        dr_ste_copy_mask_misc(buff, &set_param->misc, clear);
}

The comment and the {} initialiser are gone, as you asked.

One thing I checked before dropping the initialiser: skipping a section means set_param keeps
whatever the caller passed in, so the zero has to come from the caller. dr_rule_create_rule()
declares struct dr_match_param param = {}, and dr_matcher_copy_mask() always passes a
full-size buffer so it never takes the partial path. So the initialiser was indeed redundant, but
only because of those two call sites.

Verification, both against the compiled object:

behaviour is unchanged versus the previous version: 14351 (criteria, mask_sz) pairs
compared byte for byte, 0 differences -- this includes the mask_sz == param_location
boundary, where the section is now skipped entirely instead of being parsed from a zero
buffer;
the property still holds: over all section bits and every 4-byte mask_sz from 4 to 448, a
short mask gives a byte-identical dr_match_param to the same mask zero padded to
DEVX_ST_SZ_BYTES(dr_match_param), 14224 combinations, 0 mismatches.

Copilot AI balanced review requested due to automatic review settings October 4, 2026 14:13

Copilot AI reviewed Oct 4, 2026

alexvesker suggested changes Oct 11, 2026

View reviewed changes

ez-lbz force-pushed the fix/dr-ste-copy-param-underflow branch from c922995 to 902dfd8 Compare October 12, 2026 03:18

Copilot AI balanced review requested due to automatic review settings October 12, 2026 03:18

Copilot AI reviewed Oct 12, 2026

alexvesker reviewed Oct 12, 2026

View reviewed changes

ez-lbz force-pushed the fix/dr-ste-copy-param-underflow branch from 902dfd8 to 449134c Compare October 15, 2026 08:14

Copilot AI balanced review requested due to automatic review settings October 15, 2026 08:14

Copilot AI reviewed Oct 15, 2026

Conversation

ez-lbz commented Oct 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

ez-lbz commented Oct 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

ez-lbz commented Oct 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants