On 7/8/20 6:04 PM, Nick Desaulniers wrote:
From: Jakub Kicinski kuba@kernel.org
When ur_load_imm_any() is inlined into jeq_imm(), it's possible for the compiler to deduce a case where _val can only have the value of -1 at compile time. Specifically,
/* struct bpf_insn: _s32 imm */ u64 imm = insn->imm; /* sign extend */ if (imm >> 32) { /* non-zero only if insn->imm is negative */ /* inlined from ur_load_imm_any */ u32 __imm = imm >> 32; /* therefore, always 0xffffffff */ if (__builtin_constant_p(__imm) && __imm > 255) compiletime_assert_XXX()
This can result in tripping a BUILD_BUG_ON() in __BF_FIELD_CHECK() that checks that a given value is representable in one byte (interpreted as unsigned).
Looking at the 12 other callers of FIELD_FIT(), it's hard to tell but it appears most of them don't supply constant _val to the macro. So maybe relaxing this check is no great loss.
I feel like I need to look much more deeply into this to call it a review, so:
Acked-by: Alex Elder elder@linaro.org
FIELD_FIT() should return true or false at runtime for whether a value can fit for not. Don't break the build over a value that's too large for the mask. We'd prefer to keep the inlining and compiler optimizations though we know this case will always return false.
Cc: stable@vger.kernel.org Link: https://lore.kernel.org/kernel-hardening/CAK7LNASvb0UDJ0U5wkYYRzTAdnEs64HjXp... Reported-by: Masahiro Yamada masahiroy@kernel.org Debugged-by: Sami Tolvanen samitolvanen@google.com Signed-off-by: Jakub Kicinski kuba@kernel.org Signed-off-by: Nick Desaulniers ndesaulniers@google.com
Changes V1->V2:
- None
include/linux/bitfield.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h index 48ea093ff04c..4e035aca6f7e 100644 --- a/include/linux/bitfield.h +++ b/include/linux/bitfield.h @@ -77,7 +77,7 @@ */ #define FIELD_FIT(_mask, _val) \ ({ \
__BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_FIT: "); \
__BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: "); \