diff mbox series

[net,v2] netfilter: Add protection for bmp length out of range

Message ID d2b63acc5cd76db46132eb6ebd106f159fc5132d.camel@mediatek.com
State New
Headers
Series [net,v2] netfilter: Add protection for bmp length out of range |

Commit Message

Lena Wang (王娜) March 1, 2024, 3:12 p.m. UTC 
  From: Lena Wang <lena.wang@mediatek.com>

UBSAN load reports an exception of BRK#5515 SHIFT_ISSUE:Bitwise shifts
that are out of bounds for their data type.

vmlinux   get_bitmap(b=75) + 712
<net/netfilter/nf_conntrack_h323_asn1.c:0>
vmlinux   decode_seq(bs=0xFFFFFFD008037000, f=0xFFFFFFD008037018,
level=134443100) + 1956
<net/netfilter/nf_conntrack_h323_asn1.c:592>
vmlinux   decode_choice(base=0xFFFFFFD0080370F0, level=23843636) + 1216
<net/netfilter/nf_conntrack_h323_asn1.c:814>
vmlinux   decode_seq(f=0xFFFFFFD0080371A8, level=134443500) + 812
<net/netfilter/nf_conntrack_h323_asn1.c:576>
vmlinux   decode_choice(base=0xFFFFFFD008037280, level=0) + 1216
<net/netfilter/nf_conntrack_h323_asn1.c:814>
vmlinux   DecodeRasMessage() + 304
<net/netfilter/nf_conntrack_h323_asn1.c:833>
vmlinux   ras_help() + 684
<net/netfilter/nf_conntrack_h323_main.c:1728>
vmlinux   nf_confirm() + 188
<net/netfilter/nf_conntrack_proto.c:137>

Due to abnormal data in skb->data, the extension bitmap length
exceeds 32 when decoding ras message. Then get_bitmap uses the
length to make a shift operation. It will change into negative
after several loop.

UBSAN load can detect a negative shift as an undefined behaviour
and reports an exception.

So we should add the protection to avoid the length exceeding 32.
If it exceeds it will return out of range error and stop decoding
ras message.

Signed-off-by: Lena Wang <lena.wang@mediatek.com>
---
v2:
  - add length protecton for another get_bitmap call.
  - update commit message to trim stacktrace.
---
---
 net/netfilter/nf_conntrack_h323_asn1.c | 4 ++++
 1 file changed, 4 insertions(+)

 		*(unsigned int *)base = bmp;
@@ -589,6 +591,8 @@ static int decode_seq(struct bitstr *bs, const
struct field_t *f,
 	bmp2_len = get_bits(bs, 7) + 1;
 	if (nf_h323_error_boundary(bs, 0, bmp2_len))
 		return H323_ERROR_BOUND;
+	if (bmp2_len > 32)
+		return H323_ERROR_RANGE;
 	bmp2 = get_bitmap(bs, bmp2_len);
 	bmp |= bmp2 >> f->sz;
 	if (base)
-- 
2.18.0

Comments

Jiri Pirko March 1, 2024, 3:29 p.m. UTC | #1 
Fri, Mar 01, 2024 at 04:12:24PM CET, Lena.Wang@mediatek.com wrote:
>From: Lena Wang <lena.wang@mediatek.com>
>
>UBSAN load reports an exception of BRK#5515 SHIFT_ISSUE:Bitwise shifts
>that are out of bounds for their data type.
>
>vmlinux   get_bitmap(b=75) + 712
><net/netfilter/nf_conntrack_h323_asn1.c:0>
>vmlinux   decode_seq(bs=0xFFFFFFD008037000, f=0xFFFFFFD008037018,
>level=134443100) + 1956
><net/netfilter/nf_conntrack_h323_asn1.c:592>
>vmlinux   decode_choice(base=0xFFFFFFD0080370F0, level=23843636) + 1216
><net/netfilter/nf_conntrack_h323_asn1.c:814>
>vmlinux   decode_seq(f=0xFFFFFFD0080371A8, level=134443500) + 812
><net/netfilter/nf_conntrack_h323_asn1.c:576>
>vmlinux   decode_choice(base=0xFFFFFFD008037280, level=0) + 1216
><net/netfilter/nf_conntrack_h323_asn1.c:814>
>vmlinux   DecodeRasMessage() + 304
><net/netfilter/nf_conntrack_h323_asn1.c:833>
>vmlinux   ras_help() + 684
><net/netfilter/nf_conntrack_h323_main.c:1728>
>vmlinux   nf_confirm() + 188
><net/netfilter/nf_conntrack_proto.c:137>
>
>Due to abnormal data in skb->data, the extension bitmap length
>exceeds 32 when decoding ras message. Then get_bitmap uses the
>length to make a shift operation. It will change into negative
>after several loop.
>
>UBSAN load can detect a negative shift as an undefined behaviour
>and reports an exception.
>
>So we should add the protection to avoid the length exceeding 32.
>If it exceeds it will return out of range error and stop decoding
>ras message.
>
>Signed-off-by: Lena Wang <lena.wang@mediatek.com>

Missing "Fixes" tag, again...
Pablo Neira Ayuso March 2, 2024, 9:48 a.m. UTC | #2 
On Fri, Mar 01, 2024 at 03:12:24PM +0000, Lena Wang (王娜) wrote:
> From: Lena Wang <lena.wang@mediatek.com>
> 
> UBSAN load reports an exception of BRK#5515 SHIFT_ISSUE:Bitwise shifts
> that are out of bounds for their data type.
> 
> vmlinux   get_bitmap(b=75) + 712
> <net/netfilter/nf_conntrack_h323_asn1.c:0>
> vmlinux   decode_seq(bs=0xFFFFFFD008037000, f=0xFFFFFFD008037018,
> level=134443100) + 1956
> <net/netfilter/nf_conntrack_h323_asn1.c:592>
> vmlinux   decode_choice(base=0xFFFFFFD0080370F0, level=23843636) + 1216
> <net/netfilter/nf_conntrack_h323_asn1.c:814>
> vmlinux   decode_seq(f=0xFFFFFFD0080371A8, level=134443500) + 812
> <net/netfilter/nf_conntrack_h323_asn1.c:576>
> vmlinux   decode_choice(base=0xFFFFFFD008037280, level=0) + 1216
> <net/netfilter/nf_conntrack_h323_asn1.c:814>
> vmlinux   DecodeRasMessage() + 304
> <net/netfilter/nf_conntrack_h323_asn1.c:833>
> vmlinux   ras_help() + 684
> <net/netfilter/nf_conntrack_h323_main.c:1728>
> vmlinux   nf_confirm() + 188
> <net/netfilter/nf_conntrack_proto.c:137>
> 
> Due to abnormal data in skb->data, the extension bitmap length
> exceeds 32 when decoding ras message. Then get_bitmap uses the
> length to make a shift operation. It will change into negative
> after several loop.
> 
> UBSAN load can detect a negative shift as an undefined behaviour
> and reports an exception.
> 
> So we should add the protection to avoid the length exceeding 32.
> If it exceeds it will return out of range error and stop decoding
> ras message.
> 
> Signed-off-by: Lena Wang <lena.wang@mediatek.com>
> ---
> v2:
>   - add length protecton for another get_bitmap call.
>   - update commit message to trim stacktrace.
> ---
> ---
>  net/netfilter/nf_conntrack_h323_asn1.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/net/netfilter/nf_conntrack_h323_asn1.c
> b/net/netfilter/nf_conntrack_h323_asn1.c
> index e697a824b001..540d97715bd2 100644
> --- a/net/netfilter/nf_conntrack_h323_asn1.c
> +++ b/net/netfilter/nf_conntrack_h323_asn1.c
> @@ -533,6 +533,8 @@ static int decode_seq(struct bitstr *bs, const
> struct field_t *f,
>  	/* Get fields bitmap */
>  	if (nf_h323_error_boundary(bs, 0, f->sz))
>  		return H323_ERROR_BOUND;
> +	if (f->sz > 32)
> +		return H323_ERROR_RANGE;

Could you possibly place this in get_bitmap()? IIRC these are the only
two calls to this function.

Thanks.

>  	bmp = get_bitmap(bs, f->sz);
>  	if (base)
>  		*(unsigned int *)base = bmp;
> @@ -589,6 +591,8 @@ static int decode_seq(struct bitstr *bs, const
> struct field_t *f,
>  	bmp2_len = get_bits(bs, 7) + 1;
>  	if (nf_h323_error_boundary(bs, 0, bmp2_len))
>  		return H323_ERROR_BOUND;
> +	if (bmp2_len > 32)
> +		return H323_ERROR_RANGE;
>  	bmp2 = get_bitmap(bs, bmp2_len);
>  	bmp |= bmp2 >> f->sz;
>  	if (base)
> -- 
> 2.18.0
Florian Westphal March 2, 2024, 11:52 a.m. UTC | #3 
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > +	if (f->sz > 32)
> > +		return H323_ERROR_RANGE;
> 
> Could you possibly place this in get_bitmap()? IIRC these are the only
> two calls to this function.

How would you signal the error?  I think this patch is fine as-is.
Pablo Neira Ayuso March 4, 2024, 12:41 p.m. UTC | #4 
On Sat, Mar 02, 2024 at 12:52:41PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > +	if (f->sz > 32)
> > > +		return H323_ERROR_RANGE;
> > 
> > Could you possibly place this in get_bitmap()? IIRC these are the only
> > two calls to this function.
> 
> How would you signal the error?  I think this patch is fine as-is.

Provide the bitmap instead as parameter, but this opencode variant
also LGTM, I am probably overdoing, we can take this as is.

Thanks.
diff mbox series

Patch

diff --git a/net/netfilter/nf_conntrack_h323_asn1.c
b/net/netfilter/nf_conntrack_h323_asn1.c
index e697a824b001..540d97715bd2 100644
--- a/net/netfilter/nf_conntrack_h323_asn1.c
+++ b/net/netfilter/nf_conntrack_h323_asn1.c
@@ -533,6 +533,8 @@  static int decode_seq(struct bitstr *bs, const
struct field_t *f,
 	/* Get fields bitmap */
 	if (nf_h323_error_boundary(bs, 0, f->sz))
 		return H323_ERROR_BOUND;
+	if (f->sz > 32)
+		return H323_ERROR_RANGE;
 	bmp = get_bitmap(bs, f->sz);
 	if (base)