Jakub Kicinski [off-list ref] wrote:

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

Jenny, could you please double-check?
I don't think this report is right.

netfilter: nf_conntrack_h323: fix OOB read in decode_int() CONS case

This patch adds a boundary check in the CONS case of decode_int() to
prevent reading beyond the buffer after get_bits() returns a length
value. The fix validates that len bytes remain before calling
get_uint().

SO far I can follow LLM response.

quoted

diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
index 62aa22a07876..c972e9488e16 100644
--- a/net/netfilter/nf_conntrack_h323_asn1.c
+++ b/net/netfilter/nf_conntrack_h323_asn1.c

@@ -331,6 +331,8 @@ static int decode_int(struct bitstr *bs, const struct field_t *f,
 		if (nf_h323_error_boundary(bs, 0, 2))
 			return H323_ERROR_BOUND;
 		len = get_bits(bs, 2) + 1;
+		if (nf_h323_error_boundary(bs, len, 0))
+			return H323_ERROR_BOUND;
 		BYTE_ALIGN(bs);
 		if (base && (f->attr & DECODE)) {	/* timeToLive */
 			unsigned int v = get_uint(bs, len) + f->lb;

This patch fixes the CONS case, but the UNCO case in the same function
has the same vulnerability. In the UNCO case, get_len(bs) can return up
to 0x3FFF (16383 bytes), and the code advances bs->cur by that amount
without checking that len bytes remain:

	case UNCO:
		BYTE_ALIGN(bs);
		if (nf_h323_error_boundary(bs, 2, 0))
			return H323_ERROR_BOUND;
		len = get_len(bs);
		bs->cur += len;
		break;

The check only validates 2 bytes for get_len() itself, not the
subsequent len bytes that will be skipped. Does this need the same fix
pattern?

I don't think so.  Pointer can advance, but right after the swtich/case
there is:

358         if (nf_h323_error_boundary(bs, 0, 0))
359                 return H323_ERROR_BOUND;
360         return H323_ERROR_NONE;

So bs->cur can advance past cur->end, but then the function
returns ERROR_BOUND.

The decode_bitstr() function has a similar issue. In the SEMI case, it
calls get_len(bs) which can return up to 0x3FFF bits, then advances the
buffer by len >> 3 bytes plus partial bits without checking that
sufficient buffer space exists:

	case SEMI:
		if (nf_h323_error_boundary(bs, 2, 0))
			return H323_ERROR_BOUND;
		len = get_len(bs);

AFAICS we've checked get_len() doesn't cause oob access.

		break;
	}

	bs->cur += len >> 3;
	bs->bit = len & 7;

The check at the top only validates 2 bytes for the get_len() call. The
actual advance at the bottom can be up to 2047 bytes. Should this be
checked as well?

AFAICS it is, right after there is:
410         if (nf_h323_error_boundary(bs, 0, 0))
411                 return H323_ERROR_BOUND;

The decode_octstr() function has the same pattern in both the SEMI case
and the default case:

	case SEMI:
		BYTE_ALIGN(bs);
		if (nf_h323_error_boundary(bs, 2, 0))
			return H323_ERROR_BOUND;
		len = get_len(bs) + f->lb;
		break;
	default:		/* 2 <= Range <= 255 */
		if (nf_h323_error_boundary(bs, 0, f->sz))
			return H323_ERROR_BOUND;
		len = get_bits(bs, f->sz) + f->lb;
		BYTE_ALIGN(bs);
		break;
	}

	bs->cur += len;

Pointer increment, right after there is:

483         if (nf_h323_error_boundary(bs, 0, 0))
484                 return H323_ERROR_BOUND;

len bytes without checking. The existing checks only validate the bytes
needed to READ the length value, not the len bytes that will be
consumed. Should boundary checks be added after both break statements?

AFAICS there are such checks... :-/

Similarly, decode_bmpstr() (which handles 2-byte character strings) has
the same issue in both the BYTE case and default case:

	case BYTE:		/* Range == 256 */
		BYTE_ALIGN(bs);
		if (nf_h323_error_boundary(bs, 1, 0))
			return H323_ERROR_BOUND;
		len = (*bs->cur++) + f->lb;
		break;

We write to bs->cur, but AFAICS the write has been vetted for.

	default:		/* 2 <= Range <= 255 */
		if (nf_h323_error_boundary(bs, 0, f->sz))
			return H323_ERROR_BOUND;
		len = get_bits(bs, f->sz) + f->lb;
		BYTE_ALIGN(bs);
		break;
	}

	bs->cur += len << 1;

After computing len from packet data plus f->lb, the function advances
by len << 1 bytes (doubling for 2-byte characters) without verifying
that len << 1 bytes are available. Should this also be checked?

AFAICS it is...

510         bs->cur += len << 1;
511
512         if (nf_h323_error_boundary(bs, 0, 0))
513                 return H323_ERROR_BOUND;

So AFAICS this LLM response is bunk.