On Wed, Aug 14, 2024 at 05:34:52PM +0200, Daniel Kiper wrote:
> 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".
>
The N_() marcors are dropped in my WIP branch per Vladimir's comment.
I'll fix the type error in the next version.
> [...]
>
> > ==
> > 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...
>
I'll updatet the statement for all CIDs in the next version.
Gary Lin
> > ________________________________________________________________________________________________________
> > *** 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
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
