Re: [PATCH 2/2] MdePkg/UefiSpec.h: Ensure safe bitwise operations.

["Marvin Häuser" <Marvin.Haeuser@outlook.com>]

Hey Laszlo,

Thanks for your reply. Comments are inline.

Regards,
Marvin.

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, February 27, 2018 7:59 PM
> To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-
> devel@lists.01.org
> Cc: michael.d.kinney@intel.com; liming.gao@intel.com
> Subject: Re: [edk2] [PATCH 2/2] MdePkg/UefiSpec.h: Ensure safe bitwise
> operations.
> 
> Hi Marvin,
> 
> a request in advance: could you turn on threading (and *shallow* threading
> at that) in your git-send-email config? The way you sent these patches, they
> are not collected in a single thread; the patches are scattered across the
> archive / my edk2-devel folder.

Sorry, I will research that. :)

> 
> To the patch:
> 
> On 02/27/18 17:47, Marvin Häuser wrote:
> > As per the C standard, bit-level operations on signed integers are
> > either undefined or implementation-defined. Hence, use the BIT
> > definitions from Base.h where applicable and mark the others as
> > unsigned.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
> > ---
> >  MdePkg/Include/Uefi/UefiSpec.h | 96 ++++++++++----------
> >  1 file changed, 48 insertions(+), 48 deletions(-)
> >
> > diff --git a/MdePkg/Include/Uefi/UefiSpec.h
> > b/MdePkg/Include/Uefi/UefiSpec.h index ee016b48ded3..16abf3fce95f
> > 100644
> > --- a/MdePkg/Include/Uefi/UefiSpec.h
> > +++ b/MdePkg/Include/Uefi/UefiSpec.h
> > @@ -52,46 +52,46 @@ typedef enum {
> >  //
> >  // Bit definitions for EFI_TIME.Daylight  // -#define
> > EFI_TIME_ADJUST_DAYLIGHT  0x01
> > -#define EFI_TIME_IN_DAYLIGHT      0x02
> > +#define EFI_TIME_ADJUST_DAYLIGHT  BIT0
> > +#define EFI_TIME_IN_DAYLIGHT      BIT1
> 
> This might look nicer, but it makes no difference.

This also makes the values unsigned as per the previous patch.

> 
> #define  BIT0     0x00000001
> #define  BIT1     0x00000002
> 
> These constants have type "int" just the same.
> 
> (
> 
> Now, as a curiosity, BIT31 *does* have type "unsigned int":
> 
> #define  BIT31    0x80000000
> 
> This is because the 0x prefix enables, but does not force, the language to pick
> unsigned types for the constant, and because 0x80000000 does not fit in an
> "int". So "unsigned int" is picked.
> 
> BIT32 and the rest have type "unsigned long long" (or UINT64), so I agree the
> types that the BITxy macros cover are quite inconsistent.
> 
> )
> 
> What is the exact purpose here? Making the macro definitions look nicer, or
> giving the constants "unsigned int" type?

Making them unsigned was the point, as it's relevant for bitwise operations as also discussed regarding the 'prevent truncation' patches.

> 
> >
> >  ///
> >  /// Value definition for EFI_TIME.TimeZone.
> >  ///
> > -#define EFI_UNSPECIFIED_TIMEZONE  0x07FF
> > +#define EFI_UNSPECIFIED_TIMEZONE  0x07FFU
> >
> 
> I don't know where this constant is used, but, indeed, this change gives it
> "unsigned int" type (or UINT32, if you will).
> 
> >  //
> >  // Memory cacheability attributes
> >  //
> > -#define EFI_MEMORY_UC               0x0000000000000001ULL
> > -#define EFI_MEMORY_WC               0x0000000000000002ULL
> > -#define EFI_MEMORY_WT               0x0000000000000004ULL
> > -#define EFI_MEMORY_WB               0x0000000000000008ULL
> > -#define EFI_MEMORY_UCE              0x0000000000000010ULL
> > +#define EFI_MEMORY_UC               BIT0
> > +#define EFI_MEMORY_WC               BIT1
> > +#define EFI_MEMORY_WT               BIT2
> > +#define EFI_MEMORY_WB               BIT3
> > +#define EFI_MEMORY_UCE              BIT4
> 
> No, this is a bad idea. Pre-patch, the constants have type "unsigned long
> long", or UINT64. Post-patch, they'd have type "int" (or INT32).
> Such changes can cause tricky regressions; they would require very careful
> audit.
> 
> What is the exact purpose of this hunk?

This was just supposed to give some continuity to the definitions. If you are afraid something could break, this can be dropped entirely.

> 
> >  //
> >  // Physical memory protection attributes  //  // Note: UEFI spec 2.5
> > and following: use EFI_MEMORY_RO as write-protected physical memory
> > // protection attribute. Also, EFI_MEMORY_WP means cacheability
> attribute.
> >  //
> > -#define EFI_MEMORY_WP               0x0000000000001000ULL
> > -#define EFI_MEMORY_RP               0x0000000000002000ULL
> > -#define EFI_MEMORY_XP               0x0000000000004000ULL
> > -#define EFI_MEMORY_RO               0x0000000000020000ULL
> > +#define EFI_MEMORY_WP               BIT12
> > +#define EFI_MEMORY_RP               BIT13
> > +#define EFI_MEMORY_XP               BIT14
> > +#define EFI_MEMORY_RO               BIT17
> >  //
> >  // Physical memory persistence attribute.
> >  // The memory region supports byte-addressable non-volatility.
> >  //
> > -#define EFI_MEMORY_NV               0x0000000000008000ULL
> > +#define EFI_MEMORY_NV               BIT15
> >  //
> >  // The memory region provides higher reliability relative to other memory
> in the system.
> >  // If all memory has the same reliability, then this bit is not used.
> >  //
> > -#define EFI_MEMORY_MORE_RELIABLE    0x0000000000010000ULL
> > +#define EFI_MEMORY_MORE_RELIABLE    BIT16
> 
> Same counter-argument as for the "Memory cacheability attributes".

Same here too.

> 
> >  //
> >  // Runtime memory attribute
> >  //
> > -#define EFI_MEMORY_RUNTIME          0x8000000000000000ULL
> > +#define EFI_MEMORY_RUNTIME          BIT63
> 
> This change is valid (and a no-op, functionally).

I would probably vote for 'all or none' and drop this as well.

> 
> >
> >  ///
> >  /// Memory descriptor version number.
> > @@ -363,7 +363,7 @@ EFI_STATUS
> >  //
> >  // ConvertPointer DebugDisposition type.
> >  //
> > -#define EFI_OPTIONAL_PTR     0x00000001
> > +#define EFI_OPTIONAL_PTR     0x00000001U
> >
> >  /**
> 
> This has the documented effect (the type changes from "int" to "unsigned
> int", or INT32 -> UINT32). Did you audit all the locations where the macro is
> used? (Even a signed -> unsigned change could cause unexpected
> results.)
> 

The only locations I saw the definition used were drivers passing it unchanged and RuntimeDxe, checking with a binary AND for whether the bit is set. As this is a bit definitions supposed to be used with bit operations, I marked it as unsigned to prevent triggering implementation-defined behavior. Signed-specific behavior should be avoided in the first place. Actually, this should be BIT0 to be continuous with the other changes.

> >    Determines the new virtual address that is to be used on subsequent
> memory accesses.
> > @@ -393,20 +393,20 @@ EFI_STATUS
> >  // EVT_TIMER might be Ored with EVT_NOTIFY_WAIT or  //
> > EVT_NOTIFY_SIGNAL.
> >  //
> > -#define EVT_TIMER                         0x80000000
> > -#define EVT_RUNTIME                       0x40000000
> > -#define EVT_NOTIFY_WAIT                   0x00000100
> > -#define EVT_NOTIFY_SIGNAL                 0x00000200
> > +#define EVT_TIMER                         0x80000000U
> > +#define EVT_RUNTIME                       0x40000000U
> > +#define EVT_NOTIFY_WAIT                   0x00000100U
> > +#define EVT_NOTIFY_SIGNAL                 0x00000200U

To be continuous with my other changes, these should actually have used BITx.

> >
> > -#define EVT_SIGNAL_EXIT_BOOT_SERVICES     0x00000201
> > -#define EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE 0x60000202
> > +#define EVT_SIGNAL_EXIT_BOOT_SERVICES     0x00000201U
> > +#define EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE 0x60000202U
> >
> >  //
> >  // The event's NotifyContext pointer points to a runtime memory  //
> > address.
> >  // The event is deprecated in UEFI2.0 and later specifications.
> >  //
> > -#define EVT_RUNTIME_CONTEXT               0x20000000
> > +#define EVT_RUNTIME_CONTEXT               0x20000000U
> >
> >
> >  /**
> 
> In what contexts are these used so that they have to be, and can be,
> unsigned? (In general I agree that all such macros should expand to unsigned
> integer constants, but, as I mentioned earlier, changing them
> *now* might require an audit of all usage sites.)

This is actually not used anymore at all within the edk2 repo.

> 
> > @@ -1279,12 +1279,12 @@ EFI_STATUS
> >    OUT VOID                     **Interface
> >    );
> >
> > -#define EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL  0x00000001
> > -#define EFI_OPEN_PROTOCOL_GET_PROTOCOL        0x00000002
> > -#define EFI_OPEN_PROTOCOL_TEST_PROTOCOL       0x00000004
> > -#define EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER 0x00000008
> > -#define EFI_OPEN_PROTOCOL_BY_DRIVER           0x00000010
> > -#define EFI_OPEN_PROTOCOL_EXCLUSIVE           0x00000020
> > +#define EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL  BIT0
> > +#define EFI_OPEN_PROTOCOL_GET_PROTOCOL        BIT1
> > +#define EFI_OPEN_PROTOCOL_TEST_PROTOCOL       BIT2
> > +#define EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER BIT3
> > +#define EFI_OPEN_PROTOCOL_BY_DRIVER           BIT4
> > +#define EFI_OPEN_PROTOCOL_EXCLUSIVE           BIT5
> >
> >  /**
> 
> Right, these look nicer.
> 
> >    Queries a handle to determine if it supports a specified protocol.
> > If the protocol is supported by the @@ -1656,9 +1656,9 @@ typedef struct
> {
> >    VOID*    CapsulePtr[1];
> >  } EFI_CAPSULE_TABLE;
> >
> > -#define CAPSULE_FLAGS_PERSIST_ACROSS_RESET          0x00010000
> > -#define CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE         0x00020000
> > -#define CAPSULE_FLAGS_INITIATE_RESET                0x00040000
> > +#define CAPSULE_FLAGS_PERSIST_ACROSS_RESET          BIT16
> > +#define CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE         BIT17
> > +#define CAPSULE_FLAGS_INITIATE_RESET                BIT18
> >
> >  /**
> 
> OK.
> 
> >    Passes capsules to the firmware with both virtual and physical
> > mapping. Depending on the intended @@ -1764,12 +1764,12 @@
> EFI_STATUS
> > //  // Firmware should stop at a firmware user interface on next boot
> > //
> > -#define EFI_OS_INDICATIONS_BOOT_TO_FW_UI
> 0x0000000000000001
> > -#define EFI_OS_INDICATIONS_TIMESTAMP_REVOCATION
> 0x0000000000000002
> > -#define EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED
> 0x0000000000000004
> > -#define EFI_OS_INDICATIONS_FMP_CAPSULE_SUPPORTED
> 0x0000000000000008
> > -#define EFI_OS_INDICATIONS_CAPSULE_RESULT_VAR_SUPPORTED
> 0x0000000000000010
> > -#define EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY
> 0x0000000000000040
> > +#define EFI_OS_INDICATIONS_BOOT_TO_FW_UI                    BIT0
> > +#define EFI_OS_INDICATIONS_TIMESTAMP_REVOCATION             BIT1
> > +#define EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED
> BIT2
> > +#define EFI_OS_INDICATIONS_FMP_CAPSULE_SUPPORTED            BIT3
> > +#define EFI_OS_INDICATIONS_CAPSULE_RESULT_VAR_SUPPORTED
> BIT4
> > +#define EFI_OS_INDICATIONS_START_PLATFORM_RECOVERY          BIT6
> >
> >  //
> >  // EFI Runtime Services Table
> 
> OK
> 
> > @@ -2093,18 +2093,18 @@ typedef struct _EFI_LOAD_OPTION {  //  // EFI
> > Load Options Attributes  //
> > -#define LOAD_OPTION_ACTIVE              0x00000001
> > -#define LOAD_OPTION_FORCE_RECONNECT     0x00000002
> > -#define LOAD_OPTION_HIDDEN              0x00000008
> > -#define LOAD_OPTION_CATEGORY            0x00001F00
> > +#define LOAD_OPTION_ACTIVE              BIT0
> > +#define LOAD_OPTION_FORCE_RECONNECT     BIT1
> > +#define LOAD_OPTION_HIDDEN              BIT3
> > +#define LOAD_OPTION_CATEGORY            0x00001F00U
> 
> The first three changes are functionally no-ops, but the fourth change is
> inconsistent (it changes the type from "int" to "unsigned int"). Is that
> intentional?

The first few are not no-ops as per the first patch, making them unsigned.
The inconsistency results from the amount of BIT definitions that would be required to be OR'd together to achieve this value.

> 
> >
> > -#define LOAD_OPTION_CATEGORY_BOOT       0x00000000
> > -#define LOAD_OPTION_CATEGORY_APP        0x00000100
> > +#define LOAD_OPTION_CATEGORY_BOOT       0x00000000U
> > +#define LOAD_OPTION_CATEGORY_APP        0x00000100U
> >
> > -#define EFI_BOOT_OPTION_SUPPORT_KEY     0x00000001
> > -#define EFI_BOOT_OPTION_SUPPORT_APP     0x00000002
> > -#define EFI_BOOT_OPTION_SUPPORT_SYSPREP 0x00000010
> > -#define EFI_BOOT_OPTION_SUPPORT_COUNT   0x00000300
> > +#define EFI_BOOT_OPTION_SUPPORT_KEY     BIT0
> > +#define EFI_BOOT_OPTION_SUPPORT_APP     BIT1
> > +#define EFI_BOOT_OPTION_SUPPORT_SYSPREP BIT4
> > +#define EFI_BOOT_OPTION_SUPPORT_COUNT   0x00000300U
> >
> >  ///
> >  /// EFI Boot Key Data
> >
> 
> Similar comments as above.
> 
> Thanks!
> Laszlo