PR26437, PR26438 UBSAN: tc-cr16.c left shifts and overflows
Alan Modra
amodra@gmail.com
Sun Aug 30 13:18:31 GMT 2020
Always use unsigned constants in expressions generating masks. The
following trys mightily to avoid UB (but hits it anyway with bits=32
and 0x7fffffff << 1), and worse, for 32-bit int, 64-bit long, bits=32
doesn't generate 0xffffffff.
max = ((((1 << (bits - 1)) - 1) << 1) | 1);
results in -1, or max = 0xffffffffffffffff.
This patch fixes that problem, a possible shift exponent of -1U,
removes some dead code, and makes general tidies.
PR26437
PR26438
* config/tc-cr16.c: Include limits.h, formatting.
(CR16_PRINT): Wrap params in parentheses. Remove parens from uses
throughout file.
(getconstant): Handle zero nbits.
(print_operand): Simplify handling of index regs.
(check_range): Use int32_t variables. Correct range checks.
Note: diff -w so as to not post most of the whitespace changes
diff --git a/gas/config/tc-cr16.c b/gas/config/tc-cr16.c
index 32eda77a4a..71b23058bb 100644
--- a/gas/config/tc-cr16.c
+++ b/gas/config/tc-cr16.c
@@ -26,6 +26,12 @@
#include "opcode/cr16.h"
#include "elf/cr16.h"
+#ifdef HAVE_LIMITS_H
+#include <limits.h>
+#endif
+#ifndef CHAR_BIT
+#define CHAR_BIT 8
+#endif
/* Word is considered here as a 16-bit unsigned short int. */
#define WORD_SHIFT 16
@@ -41,7 +47,7 @@
/* Assign a number NUM, shifted by SHIFT bytes, into a location
pointed by index BYTE of array 'output_opcode'. */
-#define CR16_PRINT(BYTE, NUM, SHIFT) output_opcode[BYTE] |= (NUM << SHIFT)
+#define CR16_PRINT(BYTE, NUM, SHIFT) output_opcode[BYTE] |= (NUM) << (SHIFT)
/* Operand errors. */
typedef enum
@@ -169,7 +175,8 @@ l_cons (int nbytes)
if (exp.X_op != O_constant)
{
*input_line_pointer = '\0';
- as_bad (_("field width \"%s\" too complex for a bitfield"), hold);
+ as_bad (_("field width \"%s\" too complex for a bitfield"),
+ hold);
*input_line_pointer = ':';
demand_empty_rest_of_line ();
return;
@@ -185,8 +192,7 @@ l_cons (int nbytes)
nbytes),
width, nbytes, (BITS_PER_CHAR * nbytes));
width = BITS_PER_CHAR * nbytes;
- } /* Too big. */
-
+ }
if (width > bits_available)
{
@@ -205,7 +211,8 @@ l_cons (int nbytes)
char cache = *input_line_pointer;
*input_line_pointer = '\0';
- as_bad (_("field value \"%s\" too complex for a bitfield"), hold);
+ as_bad (_("field value \"%s\" too complex for a bitfield"),
+ hold);
*input_line_pointer = cache;
demand_empty_rest_of_line ();
return;
@@ -473,13 +480,12 @@ reset_vars (char *op)
#define SWITCH_TABLE(fix) \
((fix)->fx_addsy != NULL \
&& (fix)->fx_subsy != NULL \
- && S_GET_SEGMENT ((fix)->fx_addsy) == \
- S_GET_SEGMENT ((fix)->fx_subsy) \
- && S_GET_SEGMENT (fix->fx_addsy) != undefined_section \
&& ((fix)->fx_r_type == BFD_RELOC_CR16_NUM8 \
|| (fix)->fx_r_type == BFD_RELOC_CR16_NUM16 \
|| (fix)->fx_r_type == BFD_RELOC_CR16_NUM32 \
- || (fix)->fx_r_type == BFD_RELOC_CR16_NUM32a))
+ || (fix)->fx_r_type == BFD_RELOC_CR16_NUM32a) \
+ && S_GET_SEGMENT ((fix)->fx_addsy) != undefined_section \
+ && S_GET_SEGMENT ((fix)->fx_addsy) == S_GET_SEGMENT ((fix)->fx_subsy))
/* See whether we need to force a relocation into the output file.
This is used to force out switch and PC relative relocations when
@@ -1744,9 +1750,9 @@ getprocregp_image (int r)
static long
getconstant (long x, int nbits)
{
- /* The following expression avoids overflow if
- 'nbits' is the number of bits in 'bfd_vma'. */
- return (x & ((((1 << (nbits - 1)) - 1) << 1) | 1));
+ if ((unsigned) nbits >= sizeof (x) * CHAR_BIT)
+ return x;
+ return x & ((1UL << nbits) - 1);
}
/* Print a constant value to 'output_opcode':
@@ -1758,8 +1764,7 @@ static void
print_constant (int nbits, int shift, argument *arg)
{
unsigned long mask = 0;
-
- long constant = getconstant (arg->constant, nbits);
+ unsigned long constant = getconstant (arg->constant, nbits);
switch (nbits)
{
@@ -1778,7 +1783,7 @@ print_constant (int nbits, int shift, argument *arg)
output_opcode[0] output_opcode[1] */
CR16_PRINT (0, (constant >> WORD_SHIFT) & mask, 0);
- CR16_PRINT (1, (constant & 0xFFFF), WORD_SHIFT);
+ CR16_PRINT (1, constant & 0xFFFF, WORD_SHIFT);
break;
case 21:
@@ -1800,17 +1805,20 @@ print_constant (int nbits, int shift, argument *arg)
+---------+---------+---------+---------+
output_opcode[0] output_opcode[1] */
- if ((instruction->size > 2) && (shift == WORD_SHIFT))
+ if (instruction->size > 2 && shift == WORD_SHIFT)
{
if (arg->type == arg_idxrp)
{
CR16_PRINT (0, ((constant >> WORD_SHIFT) & mask) << 8, 0);
- CR16_PRINT (1, (constant & 0xFFFF), WORD_SHIFT);
+ CR16_PRINT (1, constant & 0xFFFF, WORD_SHIFT);
}
else
{
- CR16_PRINT (0, (((((constant >> WORD_SHIFT) & mask) << 8) & 0x0f00) | ((((constant >> WORD_SHIFT) & mask) >> 4) & 0xf)),0);
- CR16_PRINT (1, (constant & 0xFFFF), WORD_SHIFT);
+ CR16_PRINT (0,
+ ((((constant >> WORD_SHIFT) & mask & 0xf) << 8)
+ | (((constant >> WORD_SHIFT) & mask & 0xf0) >> 4)),
+ 0);
+ CR16_PRINT (1, constant & 0xFFFF, WORD_SHIFT);
}
}
else
@@ -1822,10 +1830,10 @@ print_constant (int nbits, int shift, argument *arg)
{
if (instruction->size == 2)
{
- CR16_PRINT (0, ((constant) & 0xf), shift); /* 0-3 bits. */
- CR16_PRINT (0, ((constant >> 4) & 0x3), (shift + 20)); /* 4-5 bits. */
- CR16_PRINT (0, ((constant >> 6) & 0x3), (shift + 14)); /* 6-7 bits. */
- CR16_PRINT (0, ((constant >> 8) & 0x3f), (shift + 8)); /* 8-13 bits. */
+ CR16_PRINT (0, (constant) & 0xf, shift); /* 0-3 bits. */
+ CR16_PRINT (0, (constant >> 4) & 0x3, shift + 20); /* 4-5 bits. */
+ CR16_PRINT (0, (constant >> 6) & 0x3, shift + 14); /* 6-7 bits. */
+ CR16_PRINT (0, (constant >> 8) & 0x3f, shift + 8); /* 8-13 bits. */
}
else
CR16_PRINT (0, constant, shift);
@@ -1844,15 +1852,15 @@ print_constant (int nbits, int shift, argument *arg)
+---------+---------+---------+---------+
output_opcode[0] output_opcode[1] */
- if ((instruction->size > 2) && (shift == WORD_SHIFT))
+ if (instruction->size > 2 && shift == WORD_SHIFT)
CR16_PRINT (1, constant, WORD_SHIFT);
else
CR16_PRINT (0, constant, shift);
break;
case 8:
- CR16_PRINT (0, ((constant / 2) & 0xf), shift);
- CR16_PRINT (0, ((constant / 2) >> 4), (shift + 8));
+ CR16_PRINT (0, (constant / 2) & 0xf, shift);
+ CR16_PRINT (0, (constant / 2) >> 4, shift + 8);
break;
default:
@@ -1901,34 +1909,20 @@ print_operand (int nbits, int shift, argument *arg)
if (instruction->size == 3)
{
CR16_PRINT (0, getidxregp_image (arg->rp), 0);
- if (getreg_image (arg->i_r) == 12)
- CR16_PRINT (0, 0, 3);
- else
- CR16_PRINT (0, 1, 3);
+ CR16_PRINT (0, getreg_image (arg->i_r) & 1, 3);
}
else
{
CR16_PRINT (0, getidxregp_image (arg->rp), 16);
- if (getreg_image (arg->i_r) == 12)
- CR16_PRINT (0, 0, 19);
- else
- CR16_PRINT (0, 1, 19);
+ CR16_PRINT (0, getreg_image (arg->i_r) & 1, 19);
}
print_constant (nbits, shift, arg);
break;
case arg_idxr:
- if (getreg_image (arg->i_r) == 12)
- if (IS_INSN_MNEMONIC ("cbitb") || IS_INSN_MNEMONIC ("sbitb")
- || IS_INSN_MNEMONIC ("tbitb"))
- CR16_PRINT (0, 0, 23);
- else CR16_PRINT (0, 0, 24);
- else
- if (IS_INSN_MNEMONIC ("cbitb") || IS_INSN_MNEMONIC ("sbitb")
- || IS_INSN_MNEMONIC ("tbitb"))
- CR16_PRINT (0, 1, 23);
- else CR16_PRINT (0, 1, 24);
-
+ CR16_PRINT (0, getreg_image (arg->i_r) & 1,
+ (IS_INSN_TYPE (CSTBIT_INS)
+ && instruction->mnemonic[4] == 'b') ? 23 : 24);
print_constant (nbits, shift, arg);
break;
@@ -1944,20 +1938,16 @@ print_operand (int nbits, int shift, argument *arg)
case arg_cr:
print_constant (nbits, shift, arg);
/* Add the register argument to the output_opcode. */
- CR16_PRINT (0, getreg_image (arg->r), (shift+16));
+ CR16_PRINT (0, getreg_image (arg->r), shift - 16);
break;
case arg_crp:
print_constant (nbits, shift, arg);
- if (instruction->size > 1)
- CR16_PRINT (0, getregp_image (arg->rp), (shift + 16));
- else if (IS_INSN_TYPE (LD_STOR_INS) || (IS_INSN_TYPE (CSTBIT_INS)))
- {
- if (instruction->size == 2)
- CR16_PRINT (0, getregp_image (arg->rp), (shift - 8));
- else if (instruction->size == 1)
+ if ((IS_INSN_TYPE (LD_STOR_INS) || IS_INSN_TYPE (CSTBIT_INS))
+ && instruction->size == 1)
CR16_PRINT (0, getregp_image (arg->rp), 16);
- }
+ else if (instruction->size > 1)
+ CR16_PRINT (0, getregp_image (arg->rp), (shift + 16) & 31);
else
CR16_PRINT (0, getregp_image (arg->rp), shift);
break;
@@ -1987,18 +1977,9 @@ get_number_of_operands (void)
static op_err
check_range (long *num, int bits, int unsigned flags, int update)
{
- long min, max;
+ int32_t min, max;
op_err retval = OP_LEGAL;
- long value = *num;
-
- if (bits == 0 && value > 0) return OP_OUT_OF_RANGE;
-
- /* For hosts with longs bigger than 32-bits make sure that the top
- bits of a 32-bit negative value read in by the parser are set,
- so that the correct comparisons are made. */
- if (value & 0x80000000)
- value |= (-1UL << 31);
-
+ int32_t value = *num;
/* Verify operand value is even. */
if (flags & OP_EVEN)
@@ -2051,26 +2032,30 @@ check_range (long *num, int bits, int unsigned flags, int update)
return OP_OUT_OF_RANGE;
}
+ if (bits == 0)
+ {
+ if (value != 0)
+ retval = OP_OUT_OF_RANGE;
+ return retval;
+ }
+
if (flags & OP_SIGNED)
{
- max = (1 << (bits - 1)) - 1;
- min = - (1 << (bits - 1));
- if ((value > max) || (value < min))
+ max = (1U << (bits - 1)) - 1;
+ min = - (1U << (bits - 1));
+ if (value > max || value < min)
retval = OP_OUT_OF_RANGE;
}
else if (flags & OP_UNSIGNED)
{
- max = ((((1 << (bits - 1)) - 1) << 1) | 1);
- min = 0;
- if (((unsigned long) value > (unsigned long) max)
- || ((unsigned long) value < (unsigned long) min))
+ max = (1U << (bits - 1) << 1) - 1;
+ if ((uint32_t) value > (uint32_t) max)
retval = OP_OUT_OF_RANGE;
}
else if (flags & OP_NEG)
{
- max = - 1;
- min = - ((1 << (bits - 1)) - 1);
- if ((value > max) || (value < min))
+ min = - ((1U << (bits - 1)) - 1);
+ if (value < min)
retval = OP_OUT_OF_RANGE;
}
return retval;
@@ -2090,7 +2075,8 @@ warn_if_needed (ins *insn)
/* Enough to verify that one of the arguments is a simple reg. */
if ((insn->arg[0].type == arg_r) || (insn->arg[1].type == arg_r))
if (insn->arg[0].r == insn->arg[1].r)
- as_bad (_("Same src/dest register is used (`r%d'), result is undefined"), insn->arg[0].r);
+ as_bad (_("Same src/dest register is used (`r%d'), "
+ "result is undefined"), insn->arg[0].r);
}
if (IS_INSN_MNEMONIC ("pop")
--
Alan Modra
Australia Development Lab, IBM
More information about the Binutils
mailing list