Re: [PATCH net] netfilter: nf_conntrack_h323: fix get_bitmap() overread
Pablo Neira Ayuso <[email protected]>
| Newsgroups | org.kernel.vger.netdev,org.kernel.vger.netfilter-devel,org.kernel.vger.stable |
|---|---|
| Message-ID | <aoOQHtBE1GNDaPou@chamomile> |
On Wed, Jul 22, 2026 at 09:57:14PM +0900, Sangho Lee wrote:
> get_bitmap() first consumes the complete bytes covered by the current bit
> offset and the requested bitmap length. For totals below 32 bits, it then
> unconditionally reads one more byte for the partial-byte remainder.
>
> When the total is already byte aligned, there is no remainder. The
> caller's boundary check correctly permits only the complete bytes, so the
> extra load reads one byte past the supplied PER buffer. A crafted Q.931
> User-User IE reaches this with a 17-bit sequence extension bitmap starting
> at bit offset 7. An AddressSanitizer build of the decoder reports:
>
> heap-buffer-overflow in get_bitmap
> decode_seq
> decode_seq
> DecodeH323_UserInformation
> DecodeQ931
>
> The same condition also excludes a total of exactly 32 bits from the
> alignment shift, producing an incorrect bitmap for unaligned inputs.
>
> Read a trailing byte only when there is a partial-byte remainder. Include
> the 32-bit total in the alignment branch. This makes all start offsets
> and bitmap lengths from 1 through 32 agree with a bit-by-bit reference
> decoder.
>
> The same input reaches q931_help(), DecodeQ931(), and decode_seq() through
> an nftables Q.931 conntrack helper on current nf.git. The helper's static
> scratch buffer has tailroom, so in-kernel KASAN does not report this logical
> packet-boundary overread. A diagnostic check immediately before the load
> does observe bs->cur == bs->end. No crash, disclosure, or corruption has
> been demonstrated.
>
> Fixes: 5e35941d9901 ("[NETFILTER]: Add H.323 conntrack/NAT helper")
> Cc: [email protected]
> Signed-off-by: Sangho Lee <[email protected]>
> ---
> net/netfilter/nf_conntrack_h323_asn1.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
> index 6830c9da3..482860d2c 100644
> --- a/net/netfilter/nf_conntrack_h323_asn1.c
> +++ b/net/netfilter/nf_conntrack_h323_asn1.c
> @@ -228,8 +228,9 @@ static unsigned int get_bitmap(struct bitstr *bs, unsigned int b)
> bytes--, shift -= 8)
> v |= (unsigned int)(*bs->cur++) << shift;
>
> - if (l < 32) {
> - v |= (unsigned int)(*bs->cur) << shift;
> + if (l <= 32) {
> + if (l & 7)
> + v |= (unsigned int)(*bs->cur) << shift;
> v <<= bs->bit;
> } else if (l > 32) {
This branch is now dead code after your update.
> v <<= bs->bit;
> --
> 2.43.0