[PATCH] bpf, arm32: Correct check_imm24

[PATCH] bpf, arm32: Correct check_imm24

[Wang YanQing]

imm24 is signed, so the right range is:
[-(2<<(24 - 1)), (2<<(24 - 1)) - 1]

Note:this patch also fix a typo.

Signed-off-by: Wang YanQing <udknight@gmail.com>
---
 arch/arm/net/bpf_jit_32.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
index c0b4124..19c6d77 100644
--- a/arch/arm/net/bpf_jit_32.c
+++ b/arch/arm/net/bpf_jit_32.c
@@ -84,7 +84,7 @@
  *
  * 1. First argument is passed using the arm 32bit registers and rest of the
  * arguments are passed on stack scratch space.
- * 2. First callee-saved arugument is mapped to arm 32 bit registers and rest
+ * 2. First callee-saved argument is mapped to arm 32 bit registers and rest
  * arguments are mapped to scratch space on stack.
  * 3. We need two 64 bit temp registers to do complex operations on eBPF
  * registers.
@@ -1198,15 +1198,14 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx)
 	u8 rd, rt, rm, rn;
 	s32 jmp_offset;
 
-#define check_imm(bits, imm) do {				\
-	if ((((imm) > 0) && ((imm) >> (bits))) ||		\
-	    (((imm) < 0) && (~(imm) >> (bits)))) {		\
-		pr_info("[%2d] imm=%d(0x%x) out of range\n",	\
-			i, imm, imm);				\
+#define check_imm_range(min, max, imm) do {			\
+	if (imm < min || imm > max) {				\
+		pr_info("[%2d] imm=%d is out of range\n",	\
+			i, imm);				\
 		return -EINVAL;					\
 	}							\
 } while (0)
-#define check_imm24(imm) check_imm(24, imm)
+#define check_imm24(imm) check_imm_range(-16777216, 16777215, imm)
 
 	switch (code) {
 	/* ALU operations */
-- 
1.8.5.6.2.g3d8a54e.dirty

[PATCH] bpf, arm32: Correct check_imm24

[Russell King - ARM Linux]

On Thu, May 10, 2018 at 11:20:13AM +0800, Wang YanQing wrote:
> imm24 is signed, so the right range is:
> [-(2<<(24 - 1)), (2<<(24 - 1)) - 1]

2 << (24 - 1) is the same as 1 << 24.

> -#define check_imm(bits, imm) do {				\
> -	if ((((imm) > 0) && ((imm) >> (bits))) ||		\
> -	    (((imm) < 0) && (~(imm) >> (bits)))) {		\
> -		pr_info("[%2d] imm=%d(0x%x) out of range\n",	\
> -			i, imm, imm);				\
> +#define check_imm_range(min, max, imm) do {			\
> +	if (imm < min || imm > max) {				\
> +		pr_info("[%2d] imm=%d is out of range\n",	\
> +			i, imm);				\
>  		return -EINVAL;					\
>  	}							\
>  } while (0)
> -#define check_imm24(imm) check_imm(24, imm)
> +#define check_imm24(imm) check_imm_range(-16777216, 16777215, imm)

How is this any different?

If imm is 16777216, then "imm > max" in your version is true.
In the original version, "imm > 0" is true, so we then test for
"16777216 >> 24" being non-zero.  That's also true, so the test
condition fires.

If imm is 16777215, then "imm > max" is false in your version.
In the original version, the conditions also evaluate to false.

For the -16777217 case, "imm < min" in your version is true.
In the original version, "imm < 0" is true, so we then test for
"~(-16777217) >> 24" being non-zero.  This is the same as
"16777216 >> 24" being non-zero, which is true so the condition
fires.

With -16777216, the same thing happens, both end up evaluating
to false.

So, the two cases end up producing identical results, and there
is no actual effect from this change.

However, your commit message is correct - there is a bug here.
That's obvious when you mask the "imm" value with 0x00ffffff,
and realise that an imm value of -16777216 ends up having the
same value in the instruction as an imm value of 0.  So, the
range of "imm" is _half_ that.

 #define check_imm(bits, imm) do {				\
-	if ((((imm) > 0) && ((imm) >> (bits))) ||		\
-	    (((imm) < 0) && (~(imm) >> (bits)))) {		\
+	if ((((imm) > 0) && ((imm) >> (bits - 1))) ||		\
+	    (((imm) < 0) && (~(imm) >> (bits - 1)))) {		\
 		pr_info("[%2d] imm=%d(0x%x) out of range\n",	\
 			i, imm, imm);				\

would fix it.  Alternatively:

 #define check_imm(bits, imm) do {				\
-	if ((((imm) > 0) && ((imm) >> (bits))) ||		\
-	    (((imm) < 0) && (~(imm) >> (bits)))) {		\
+	if ((imm) >= (1 << ((bits) - 1)) ||			\
+	    (imm) < -(1 << ((bits) - 1))) {			\
 		pr_info("[%2d] imm=%d(0x%x) out of range\n",	\
 			i, imm, imm);				\

would also fix it.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync@8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

[PATCH] bpf, arm32: Correct check_imm24

[Wang YanQing]

On Thu, May 10, 2018 at 08:56:57AM +0100, Russell King - ARM Linux wrote:
> On Thu, May 10, 2018 at 11:20:13AM +0800, Wang YanQing wrote:
> > imm24 is signed, so the right range is:
> > [-(2<<(24 - 1)), (2<<(24 - 1)) - 1]
> 
> 2 << (24 - 1) is the same as 1 << 24.
> 
> > -#define check_imm(bits, imm) do {				\
> > -	if ((((imm) > 0) && ((imm) >> (bits))) ||		\
> > -	    (((imm) < 0) && (~(imm) >> (bits)))) {		\
> > -		pr_info("[%2d] imm=%d(0x%x) out of range\n",	\
> > -			i, imm, imm);				\
> > +#define check_imm_range(min, max, imm) do {			\
> > +	if (imm < min || imm > max) {				\
> > +		pr_info("[%2d] imm=%d is out of range\n",	\
> > +			i, imm);				\
> >  		return -EINVAL;					\
> >  	}							\
> >  } while (0)
> > -#define check_imm24(imm) check_imm(24, imm)
> > +#define check_imm24(imm) check_imm_range(-16777216, 16777215, imm)
> 
> How is this any different?
> 
> If imm is 16777216, then "imm > max" in your version is true.
> In the original version, "imm > 0" is true, so we then test for
> "16777216 >> 24" being non-zero.  That's also true, so the test
> condition fires.
> 
> If imm is 16777215, then "imm > max" is false in your version.
> In the original version, the conditions also evaluate to false.
> 
> For the -16777217 case, "imm < min" in your version is true.
> In the original version, "imm < 0" is true, so we then test for
> "~(-16777217) >> 24" being non-zero.  This is the same as
> "16777216 >> 24" being non-zero, which is true so the condition
> fires.
> 
> With -16777216, the same thing happens, both end up evaluating
> to false.
> 
> So, the two cases end up producing identical results, and there
> is no actual effect from this change.
> 
> However, your commit message is correct - there is a bug here.
> That's obvious when you mask the "imm" value with 0x00ffffff,
> and realise that an imm value of -16777216 ends up having the
> same value in the instruction as an imm value of 0.  So, the
> range of "imm" is _half_ that.
> 
>  #define check_imm(bits, imm) do {				\
> -	if ((((imm) > 0) && ((imm) >> (bits))) ||		\
> -	    (((imm) < 0) && (~(imm) >> (bits)))) {		\
> +	if ((((imm) > 0) && ((imm) >> (bits - 1))) ||		\
> +	    (((imm) < 0) && (~(imm) >> (bits - 1)))) {		\
>  		pr_info("[%2d] imm=%d(0x%x) out of range\n",	\
>  			i, imm, imm);				\
> 
> would fix it.  Alternatively:
> 
>  #define check_imm(bits, imm) do {				\
> -	if ((((imm) > 0) && ((imm) >> (bits))) ||		\
> -	    (((imm) < 0) && (~(imm) >> (bits)))) {		\
> +	if ((imm) >= (1 << ((bits) - 1)) ||			\
> +	    (imm) < -(1 << ((bits) - 1))) {			\
>  		pr_info("[%2d] imm=%d(0x%x) out of range\n",	\
>  			i, imm, imm);				\
> 
> would also fix it.

Hi!

Sorry for confusion, I make a mistake here, the real fix I want to
submit is [8388607, -8388608], this range has the same effect as your
suggestion.

Will you fix it? or I resend another version?

Thanks.