| Message ID | 1403516340-22997-15-git-send-email-markos.chandras@imgtec.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Markos Chandras > Remove BUG_ON() if the shift immediate is >=32 to avoid > kernel crashes due to malicious user input. Since the micro-assembler > will not allow an immediate greater or equal to 32, we will use the > maximum value which is 31. This will do the correct thing on either 32- > or 64-bit cores since no 64-bit instructions are being used in JIT. I'm not sure that bounding the shift to 31 bits 'is the correct thing'. I'd have thought that emulating the large shift or masking the shift to 5 bits are equally 'correct'. ... > { > /* sa is 5-bits long */ > - BUG_ON(sa >= BIT(5)); > + if (sa >= BIT(5)) > + sa = BIT(5) - 1; > emit_instr(ctx, sll, dst, src, sa); ... David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/23/2014 10:44 AM, David Laight wrote: > From: Markos Chandras >> Remove BUG_ON() if the shift immediate is >=32 to avoid >> kernel crashes due to malicious user input. Since the micro-assembler >> will not allow an immediate greater or equal to 32, we will use the >> maximum value which is 31. This will do the correct thing on either 32- >> or 64-bit cores since no 64-bit instructions are being used in JIT. > > I'm not sure that bounding the shift to 31 bits 'is the correct thing'. > I'd have thought that emulating the large shift or masking the shift > to 5 bits are equally 'correct'. > > ... Hi David, Since we use 32-bit registers (or rather, we ignore the top 32bits on MIPS64), shifting >= 32 will always result to 0. Alexei suggested [1] to allow large shifts and emulate them, so this patch aims to do that by treating >=32 shift values as 31. Please tell me if I got this wrong. [1] http://www.linux-mips.org/archives/linux-mips/2014-06/msg00212.html
From: Markos Chandras > On 06/23/2014 10:44 AM, David Laight wrote: > > From: Markos Chandras > >> Remove BUG_ON() if the shift immediate is >=32 to avoid > >> kernel crashes due to malicious user input. Since the micro-assembler > >> will not allow an immediate greater or equal to 32, we will use the > >> maximum value which is 31. This will do the correct thing on either 32- > >> or 64-bit cores since no 64-bit instructions are being used in JIT. > > > > I'm not sure that bounding the shift to 31 bits 'is the correct thing'. > > I'd have thought that emulating the large shift or masking the shift > > to 5 bits are equally 'correct'. > > > > ... > Hi David, > > Since we use 32-bit registers (or rather, we ignore the top 32bits on > MIPS64), shifting >= 32 will always result to 0. > Alexei suggested [1] to allow large shifts and emulate them, so this > patch aims to do that by treating >=32 shift values as 31. Please tell > me if I got this wrong. Shifting by 31 converts 0xffffffff to 1, not 0. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/mips/net/bpf_jit.c b/arch/mips/net/bpf_jit.c index 1bcd599d9971..09ebc886c7aa 100644 --- a/arch/mips/net/bpf_jit.c +++ b/arch/mips/net/bpf_jit.c @@ -309,7 +309,8 @@ static inline void emit_sll(unsigned int dst, unsigned int src, unsigned int sa, struct jit_ctx *ctx) { /* sa is 5-bits long */ - BUG_ON(sa >= BIT(5)); + if (sa >= BIT(5)) + sa = BIT(5) - 1; emit_instr(ctx, sll, dst, src, sa); } @@ -323,7 +324,8 @@ static inline void emit_srl(unsigned int dst, unsigned int src, unsigned int sa, struct jit_ctx *ctx) { /* sa is 5-bits long */ - BUG_ON(sa >= BIT(5)); + if (sa >= BIT(5)) + sa = BIT(5) - 1; emit_instr(ctx, srl, dst, src, sa); }
Remove BUG_ON() if the shift immediate is >=32 to avoid kernel crashes due to malicious user input. Since the micro-assembler will not allow an immediate greater or equal to 32, we will use the maximum value which is 31. This will do the correct thing on either 32- or 64-bit cores since no 64-bit instructions are being used in JIT. Cc: "David S. Miller" <davem@davemloft.net> Cc: Daniel Borkmann <dborkman@redhat.com> Cc: Alexei Starovoitov <ast@plumgrid.com> Cc: netdev@vger.kernel.org Signed-off-by: Markos Chandras <markos.chandras@imgtec.com> --- arch/mips/net/bpf_jit.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)