On Fri, Jun 28, 2024 at 04:18:43PM +0800, Gary Lin via Grub-devel wrote:
> GIT repo for v18: https://github.com/lcp/grub2/tree/tpm2-unlock-v18
>
> This patch series is based on "Automatic TPM Disk Unlock"(*1) posted by
> Hernan Gatta to introduce the key protector framework and TPM2 stack
> to GRUB2, and this could be a useful feature for the systems to
> implement full disk encryption.
>
> To support TPM 2.0 Key File format(*2), patch 1~6,8-10 are grabbed from
> Daniel Axtens's "appended signature secure boot support" (*3) to import
> libtasn1 into grub2. Besides, the libtasn1 version is upgraded to
> 4.19.0 instead of 4.16.0 in the original patch.
Build of this version fails in the following way:
arm-linux-gnueabihf-gcc -DHAVE_CONFIG_H -I. -I.. -Wall -W
-DGRUB_MACHINE_EFI=1 -DGRUB_MACHINE=ARM_EFI -mword-relocations -nostdinc
-isystem /usr/lib/gcc-cross/arm-linux-gnueabihf/6/include -I../include
-I../include -DGRUB_FILE=\"commands/tpm2_key_protector/args.c\" -I. -I. -I..
-I.. -I../include -I../include -I../grub-core/lib/libgcrypt-grub/src/
-I./lib/tss2 -I./lib/libtasn1-grub -D_FILE_OFFSET_BITS=64 -std=gnu99
-fno-common -Os -Wall -W -Wshadow -Wpointer-arith -Wundef -Wchar-subscripts
-Wcomment -Wdeprecated-declarations -Wdisabled-optimization -Wdiv-by-zero
-Wfloat-equal -Wformat-extra-args -Wformat-security -Wformat-y2k -Wimplicit
-Wimplicit-function-declaration -Wimplicit-int -Wmain -Wmissing-braces
-Wmissing-format-attribute -Wmultichar -Wparentheses -Wreturn-type
-Wsequence-point -Wshadow -Wsign-compare -Wswitch -Wtrigraphs -Wunknown-pragmas
-Wunused -Wunused-function -Wunused-label -Wunused-parameter -Wunused-value
-Wunused-variable -Wwrite-strings -Wnested-externs -Wstrict-prototypes -g
-Wredundant-decls -Wmissing-prototypes -Wmissing-declarations -Wcast-align
-Wextra -Wattributes -Wendif-labels -Winit-self -Wint-to-pointer-cast
-Winvalid-pch -Wmissing-field-initializers -Wnonnull -Woverflow -Wvla
-Wpointer-to-int-cast -Wstrict-aliasing -Wvariadic-macros
-Wvolatile-register-var -Wpointer-sign -Wmissing-include-dirs
-Wmissing-prototypes -Wmissing-declarations -Wformat=2 -freg-struct-return
-msoft-float -fno-omit-frame-pointer -fno-dwarf2-cfi-asm
-fno-asynchronous-unwind-tables -fno-unwind-tables -fno-ident -mthumb-interwork
-fno-PIE -fno-pie -fno-stack-protector -mno-unaligned-access -Wtrampolines
-Werror -ffreestanding -MT
commands/tpm2_key_protector/tpm2_key_protector_module-args.o -MD -MP -MF
commands/tpm2_key_protector/.deps-core/tpm2_key_protector_module-args.Tpo -c -o
commands/tpm2_key_protector/tpm2_key_protector_module-args.o `test -f
'commands/tpm2_key_protector/args.c' || echo
'./'`commands/tpm2_key_protector/args.c
In file included from ../include/grub/misc.h:27:0,
from commands/tpm2_key_protector/args.c:22:
commands/tpm2_key_protector/args.c: In function
‘grub_tpm2_protector_parse_pcrs’:
commands/tpm2_key_protector/args.c:54:47: error: format ‘%lu’ expects
argument of type ‘long unsigned int’, but argument 3 has type ‘grub_uint64_t
{aka long long unsigned int}’ [-Werror=format=]
return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("Entry %lu in PCR list is too
large to be a PCR number, PCR numbers range from 0 to %u"), pcr, TPM_MAX_PCRS);
^
../include/grub/i18n.h:66:17: note: in definition of macro ‘N_’
#define N_(str) str
^~~
commands/tpm2_key_protector/args.c: In function
‘grub_tpm2_protector_parse_tpm_handle’:
commands/tpm2_key_protector/args.c:125:50: error: format ‘%lu’ expects
argument of type ‘long unsigned int’, but argument 3 has type ‘grub_uint64_t
{aka long long unsigned int}’ [-Werror=format=]
return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("Value %lu is too large to
be a TPM handle, TPM handles are unsigned 32-bit integers"), num);
^
../include/grub/i18n.h:66:17: note: in definition of macro ‘N_’
#define N_(str) str
^~~
I think you can drop N_() macros here and use PRIuGRUB_UINT64_T instead of
"%lu".
[...]
> ==
> 7 Integer handling issues:
> CID 435774, CID 435773, CID 435772, CID 435768, CID 435765, CID 435764, CID
> 435763
>
> Those are related to Gnulib macros and false positive.
> Here are the analyses:
>
> ________________________________________________________________________________________________________
> *** CID 435774: Integer handling issues (CONSTANT_EXPRESSION_RESULT)
> /grub-core/lib/libtasn1/lib/decoding.c: 481 in asn1_get_object_id_der()
> 475 */
> 476 if (leading != 0 && der[len_len + k] == 0x80)
> 477 return ASN1_DER_ERROR;
> 478 leading = 0;
> 479
> 480 /* check for wrap around */
> >>> CID 435774: Integer handling issues (CONSTANT_EXPRESSION_RESULT)
> >>> "val < ((((1 ? 0 : val) - 1 < 0) ? ~((((1 ? 0 : val) + 1 << 62UL /*
> >>> sizeof (+val) * 8 - 2 */) - 1) * 2 + 1) : ((1 ? 0 : val) + 0)) >> 7)" is
> >>> always false regardless of the values of its operands. This occurs as the
> >>> second operand of "?:".
> 481 if (INT_LEFT_SHIFT_OVERFLOW (val, 7))
> 482 return ASN1_DER_ERROR;
> 483
> 484 val = val << 7;
> 485 val |= der[len_len + k] & 0x7F;
> 486
>
> /grub-core/lib/libtasn1/lib/decoding.c: 481 in asn1_get_object_id_der()
>
> Here are the related macros:
>
> #define _GL_INT_NEGATE_CONVERT(e, v) ((1 ? 0 : (e)) - (v))
>
> #define EXPR_SIGNED(e) (_GL_INT_NEGATE_CONVERT (e, 1) < 0)
>
> #define _GL_INT_CONVERT(e, v) ((1 ? 0 : (e)) + (v))
>
> #define TYPE_WIDTH(t) (sizeof (t) * CHAR_BIT)
>
> #define _GL_SIGNED_INT_MAXIMUM(e) \
> (((_GL_INT_CONVERT (e, 1) << (TYPE_WIDTH (+ (e)) - 2)) - 1) * 2 + 1)
>
> #define _GL_INT_MINIMUM(e) \
> (EXPR_SIGNED (e) \
> ? ~ _GL_SIGNED_INT_MAXIMUM (e) \
> : _GL_INT_CONVERT (e, 0))
>
> #define _GL_INT_MAXIMUM(e) \
> (EXPR_SIGNED (e) \
> ? _GL_SIGNED_INT_MAXIMUM (e) \
> : _GL_INT_NEGATE_CONVERT (e, 1))
>
> #define INT_LEFT_SHIFT_RANGE_OVERFLOW(a, b, min, max) \
> ((a) < 0 \
> ? (a) < (min) >> (b) \
> : (max) >> (b) < (a))
>
> #define INT_LEFT_SHIFT_OVERFLOW(a, b) \
> INT_LEFT_SHIFT_RANGE_OVERFLOW (a, b, \
> _GL_INT_MINIMUM (a), _GL_INT_MAXIMUM (a))
>
> The statement in question is expanded "(a) < (min) >> (b)" of
> INT_LEFT_SHIFT_RANGE_OVERFLOW.
>
> '(a) < (min) >> (b)'
> => '(val) < _GL_INT_MINIMUM (val) >> (7)'
> => '(val) < \
> (EXPR_SIGNED (val) \
> ? ~ _GL_SIGNED_INT_MAXIMUM (val) \
> : _GL_INT_CONVERT (val, 0)) \
> >> (7)'
> => '(val) < \
> ((_GL_INT_NEGATE_CONVERT (val, 1) < 0) \
> ? ~ _GL_SIGNED_INT_MAXIMUM (val) \
> : _GL_INT_CONVERT (val, 0)) \
> >> (7)'
> => '(val) < \
> ((((1 ? 0 : (val)) - (1)) < 0) \
> ? ~ _GL_SIGNED_INT_MAXIMUM (val) \
> : _GL_INT_CONVERT (val, 0)) \
> >> (7)'
> => '(val) < \
> ((((1 ? 0 : (val)) - (1)) < 0) \
> ? ~ (((_GL_INT_CONVERT (val, 1) << (TYPE_WIDTH (+ (val)) - 2)) - 1) * 2
> + 1) \
> : _GL_INT_CONVERT (val, 0)) \
> >> (7)'
> => '(val) < \
> ((((1 ? 0 : (val)) - (1)) < 0) \
> ? ~ (((((1 ? 0 : (val)) + (1)) << (TYPE_WIDTH (+ (val)) - 2)) - 1) * 2
> + 1) \
> : _GL_INT_CONVERT (val, 0)) \
> >> (7)'
> => '(val) < \
> ((((1 ? 0 : (val)) - (1)) < 0) \
> ? ~ (((((1 ? 0 : (val)) + (1)) << ((sizeof (val) * CHAR_BIT) - 2)) - 1)
> * 2 + 1) \
> : _GL_INT_CONVERT (val, 0)) \
> >> (7)'
> => '(val) < \
> ((((1 ? 0 : (val)) - (1)) < 0) \
> ? ~ (((((1 ? 0 : (val)) + (1)) << ((sizeof (val) * CHAR_BIT) - 2)) - 1)
> * 2 + 1) \
> : ((1 ? 0 : (val)) + (0))) \
> >> (7)'
>
> '_GL_INT_MINIMUM' returns the minimum value of the given type. Since 'val' is
> 'uint64_t', '_GL_INT_MINIMUM (val)' is 0, thus '(val) < 0 >> (7)' is always
> false.
> Besides, '(val) < 0' is false, so "(max) >> (b) < (a))" becomes the only valid
> statement to check if 'a' overflows after left shift.
... but this final statement does not explain why the CID can be ignored.
AIUI simply we do not care about second operand of "?:" in this case...
> ________________________________________________________________________________________________________
> *** CID 435773: Integer handling issues (NO_EFFECT)
> /grub-core/lib/libtasn1/lib/decoding.c: 439 in asn1_get_object_id_der()
> 433 return ASN1_DER_ERROR;
> 434
> 435 val0 = 0;
> 436
> 437 for (k = 0; k < len; k++)
> 438 {
> >>> CID 435773: Integer handling issues (NO_EFFECT)
> >>> This less-than-zero comparison of an unsigned value is never true.
> >>> "(1 ? 0UL : val0) - 1UL < 0UL".
> 439 if (INT_LEFT_SHIFT_OVERFLOW (val0, 7))
> 440 return ASN1_DER_ERROR;
> 441
> 442 val0 <<= 7;
> 443 val0 |= der[len_len + k] & 0x7F;
> 444 if (!(der[len_len + k] & 0x80))
>
> Here are the related macros:
>
> #define _GL_INT_NEGATE_CONVERT(e, v) ((1 ? 0 : (e)) - (v))
>
> #define EXPR_SIGNED(e) (_GL_INT_NEGATE_CONVERT (e, 1) < 0)
>
> #define _GL_INT_CONVERT(e, v) ((1 ? 0 : (e)) + (v))
>
> #define TYPE_WIDTH(t) (sizeof (t) * CHAR_BIT)
>
> #define _GL_SIGNED_INT_MAXIMUM(e) \
> (((_GL_INT_CONVERT (e, 1) << (TYPE_WIDTH (+ (e)) - 2)) - 1) * 2 + 1)
>
> #define _GL_INT_MINIMUM(e) \
> (EXPR_SIGNED (e) \
> ? ~ _GL_SIGNED_INT_MAXIMUM (e) \
> : _GL_INT_CONVERT (e, 0))
>
> #define _GL_INT_MAXIMUM(e) \
> (EXPR_SIGNED (e) \
> ? _GL_SIGNED_INT_MAXIMUM (e) \
> : _GL_INT_NEGATE_CONVERT (e, 1))
>
> #define INT_LEFT_SHIFT_RANGE_OVERFLOW(a, b, min, max) \
> ((a) < 0 \
> ? (a) < (min) >> (b) \
> : (max) >> (b) < (a))
>
> #define INT_LEFT_SHIFT_OVERFLOW(a, b) \
> INT_LEFT_SHIFT_RANGE_OVERFLOW (a, b, \
> _GL_INT_MINIMUM (a), _GL_INT_MAXIMUM (a))
>
> The statement in question is the expanded 'EXPR_SIGNED (val0)' from either
> '_GL_INT_MAXIMUM' or '_GL_INT_MINIMUM'
>
> 'EXPR_SIGNED (val0)'
> => '(_GL_INT_NEGATE_CONVERT (val0, 1) < 0)'
> => '(((1 ? 0 : (val0)) - (1)) < 0)'
>
> Since 'EXPR_SIGNED' is to test if the given expression is signed or not. For
> 'uint64_t val0', it's expected to be false, and '_GL_INT_MINIMUM' returns the
> minimum value of 'uint64_t' while '_GL_INT_MAXIMUM' returns the maximum
> value of 'uint64_t'.
This explanation is missing final statement why this CID can be ignored.
Please fix this.
> ________________________________________________________________________________________________________
> *** CID 435772: Integer handling issues (NO_EFFECT)
> /grub-core/lib/libtasn1/lib/decoding.c: 204 in asn1_get_tag_der()
> 198 /* Long form */
> 199 punt = 1;
> 200 ris = 0;
> 201 while (punt < der_len && der[punt] & 128)
> 202 {
> 203
> >>> CID 435772: Integer handling issues (NO_EFFECT)
> >>> This less-than-zero comparison of an unsigned value is never true.
> >>> "(1 ? 0U : ((1 ? 0U : ris) + 128U)) - 1U < 0U".
> 204 if (INT_MULTIPLY_OVERFLOW (ris, 128))
> 205 return ASN1_DER_ERROR;
> 206 ris *= 128;
> 207
> 208 if (INT_ADD_OVERFLOW (ris, ((unsigned) (der[punt] & 0x7F))))
> 209 return ASN1_DER_ERROR;
>
> Here are the related macros:
>
> #define _GL_INT_NEGATE_CONVERT(e, v) ((1 ? 0 : (e)) - (v))
>
> #define _GL_INT_CONVERT(e, v) ((1 ? 0 : (e)) + (v))
>
> #define EXPR_SIGNED(e) (_GL_INT_NEGATE_CONVERT (e, 1) < 0)
>
> #define TYPE_WIDTH(t) (sizeof (t) * CHAR_BIT)
>
> #define _GL_SIGNED_INT_MAXIMUM(e) \
> (((_GL_INT_CONVERT (e, 1) << (TYPE_WIDTH (+ (e)) - 2)) - 1) * 2 + 1)
>
> #define _GL_INT_MINIMUM(e) \
> (EXPR_SIGNED (e) \
> ? ~ _GL_SIGNED_INT_MAXIMUM (e) \
> : _GL_INT_CONVERT (e, 0))
>
> #define _GL_INT_MAXIMUM(e) \
> (EXPR_SIGNED (e) \
> ? _GL_SIGNED_INT_MAXIMUM (e) \
> : _GL_INT_NEGATE_CONVERT (e, 1))
>
> #define INT_MULTIPLY_RANGE_OVERFLOW(a, b, min, max) \
> ((b) < 0 \
> ? ((a) < 0 \
> ? (a) < (max) / (b) \
> : (b) == -1 \
> ? 0 \
> : (min) / (b) < (a)) \
> : (b) == 0 \
> ? 0 \
> : ((a) < 0 \
> ? (a) < (min) / (b) \
> : (max) / (b) < (a)))
>
> # define _GL_MULTIPLY_OVERFLOW(a, b, min, max) \
> (((min) == 0 && (((a) < 0 && 0 < (b)) || ((b) < 0 && 0 < (a)))) \
> || INT_MULTIPLY_RANGE_OVERFLOW (a, b, min, max))
>
> #define _GL_BINARY_OP_OVERFLOW(a, b, op_result_overflow) \
> op_result_overflow (a, b, \
> _GL_INT_MINIMUM (_GL_INT_CONVERT (a, b)), \
> _GL_INT_MAXIMUM (_GL_INT_CONVERT (a, b)))
>
> #define INT_MULTIPLY_OVERFLOW(a, b) \
> _GL_BINARY_OP_OVERFLOW (a, b, _GL_MULTIPLY_OVERFLOW)
>
> The statement in question is the expanded 'EXPR_SIGNED (_GL_INT_CONVERT (ris,
> 128))'
> from either '_GL_INT_MINIMUM' or '_GL_INT_MAXIMUM'.
>
> 'EXPR_SIGNED (_GL_INT_CONVERT (ris, 128))'
> => 'EXPR_SIGNED ((1 ? 0 : (ris)) + (128))'
> => '(_GL_INT_NEGATE_CONVERT (((1 ? 0 : (ris)) + (128), 1) < 0))'
> => '((1 ? 0 : ((1 ? 0 : ris) + 128)) - 1 < 0)'
>
> Since 'ris' is 'unsigned int', the compiler treats 128 as 'unsigned int' too.
> 'EXPR_SIGNED' is designed to check if the given expression is signed or not.
> Thus, the result is expected to be false and '_GL_INT_MINIMUM' returns the
> minimum value of 'unsigned int' while '_GL_INT_MAXIMUM' returns the maximum
> value of 'unsigned int'.
Ditto. In general I have a feeling that your descriptions require some
rephrasing to make them clear. Same comment applies to descriptions
below...
> ________________________________________________________________________________________________________
> *** CID 435768: (CONSTANT_EXPRESSION_RESULT)
> /grub-core/lib/libtasn1/lib/decoding.c: 204 in asn1_get_tag_der()
> 198 /* Long form */
> 199 punt = 1;
> 200 ris = 0;
> 201 while (punt < der_len && der[punt] & 128)
> 202 {
> 203
> >>> CID 435768: (CONSTANT_EXPRESSION_RESULT)
> >>> "ris < (((1 ? 0 : ((1 ? 0 : ris) + 128)) - 1 < 0) ? ~((((1 ? 0 : ((1
> >>> ? 0 : ris) + 128)) + 1 << 30UL /* sizeof (+((1 ? 0 : ris) + 128)) * 8 - 2
> >>> */) - 1) * 2 + 1) : ((1 ? 0 : ((1 ? 0 : ris) + 128)) + 0)) / 128" is
> >>> always false regardless of the values of its operands. This occurs as the
> >>> second operand of "?:".
[...]
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
