| Message ID | 20210622025950.54276-1-dja@axtens.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | secvar/backend: use endian-aware types in edk2.h | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (1cdde9466ab658fb5b5a53af8c4e6a8929eef698) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
Excerpts from Daniel Axtens's message of June 22, 2021 12:59 pm: > Recently we had an issue where we did the following: > > uint16_t year = le32_to_cpu(timestamp->year); > > This is wrong and will break on BE. However, we didn't catch this > with sparse because there was a whole slew of warnings. > > The reason for the slew of warnings is that we didn't annotate the > types that store little-endian specific data in edk2.h. > > Provide the appropriate annotations. > > We now get a single sparse warning for the file, which correctly > identifies the issue: > > edk2-compat-process.c:374:46: warning: incorrect type in argument 1 (different base types) > edk2-compat-process.c:374:46: expected restricted leint32_t [usertype] le_val > edk2-compat-process.c:374:46: got restricted leint16_t const [usertype] year > > There's one annotation that I'm not super happy about - the > annotation of efi_time->timezone. Happy for other ideas on that > one. Is timezone used anywhere? > > Signed-off-by: Daniel Axtens <dja@axtens.net> > --- > libstb/secvar/backend/edk2.h | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/libstb/secvar/backend/edk2.h b/libstb/secvar/backend/edk2.h > index ef6d7c79e7ff..17df2b7bc536 100644 > --- a/libstb/secvar/backend/edk2.h > +++ b/libstb/secvar/backend/edk2.h > @@ -125,15 +125,15 @@ static const uuid_t EFI_CERT_RSA2048_GUID = {{ 0xe8, 0x66, 0x57, 0x3c, 0x9c, 0x2 > * TimeZone: -1440 to 1440 or 2047 > */ > struct efi_time { > - u16 year; > + le16 year; > u8 month; > u8 day; > u8 hour; > u8 minute; > u8 second; > u8 pad1; > - u32 nanosecond; > - s16 timezone; > + le32 nanosecond; > + s16 ENDIAN_TYPE timezone; > u8 daylight; > u8 pad2; > }; Why does it need to be annotated? None of the Linux tree's efi structs seem to have any endian annotations, is that just because they're only used on little endian platforms I wonder? Thanks, Nick
Nicholas Piggin <npiggin@gmail.com> writes: >> There's one annotation that I'm not super happy about - the >> annotation of efi_time->timezone. Happy for other ideas on that >> one. > > Is timezone used anywhere? No. I could kill off the annotation but it seems silly not to mark that it's a little-endian s16 given that I'm marking everything else. I guess we could make all the unused fields into an array of reserved u8s if we really wanted. >> >> Signed-off-by: Daniel Axtens <dja@axtens.net> >> --- >> libstb/secvar/backend/edk2.h | 18 +++++++++--------- >> 1 file changed, 9 insertions(+), 9 deletions(-) >> >> diff --git a/libstb/secvar/backend/edk2.h b/libstb/secvar/backend/edk2.h >> index ef6d7c79e7ff..17df2b7bc536 100644 >> --- a/libstb/secvar/backend/edk2.h >> +++ b/libstb/secvar/backend/edk2.h >> @@ -125,15 +125,15 @@ static const uuid_t EFI_CERT_RSA2048_GUID = {{ 0xe8, 0x66, 0x57, 0x3c, 0x9c, 0x2 >> * TimeZone: -1440 to 1440 or 2047 >> */ >> struct efi_time { >> - u16 year; >> + le16 year; >> u8 month; >> u8 day; >> u8 hour; >> u8 minute; >> u8 second; >> u8 pad1; >> - u32 nanosecond; >> - s16 timezone; >> + le32 nanosecond; >> + s16 ENDIAN_TYPE timezone; >> u8 daylight; >> u8 pad2; >> }; > > Why does it need to be annotated? None of the Linux tree's efi structs > seem to have any endian annotations, is that just because they're only > used on little endian platforms I wonder? I didn't check Linux's ones, that's interesting to know. The UEFI spec explictly states that UEFI machines are little-endian machines (s1.8.1 in v2.8), even when the underlying chip is capable of running in either endianness, so that might explain it. The rationale for changing them here is that if we don't change them, every time we do u16 year = le16_to_cpu(time->year); we get a sparse warning saying that you can't do an endian conversion on a non-endian-marked type. I suspect Linux EFI code doesn't bother with the conversion and therefore we don't get the sparse warnings on Linux. However, even if we were OK with skiboot being LE forever, we don't want to get rid of the endianness conversions here, because this code is also shared with secvarctl (and hopefully other things soon), and they can run as BE and so do need to get endianness right. Kind regards, Daniel > > Thanks, > Nick
Excerpts from Daniel Axtens's message of June 23, 2021 12:36 am: > Nicholas Piggin <npiggin@gmail.com> writes: > >>> There's one annotation that I'm not super happy about - the >>> annotation of efi_time->timezone. Happy for other ideas on that >>> one. >> >> Is timezone used anywhere? > > No. I could kill off the annotation but it seems silly not to mark that > it's a little-endian s16 given that I'm marking everything else. I guess > we could make all the unused fields into an array of reserved u8s if we > really wanted. > >>> >>> Signed-off-by: Daniel Axtens <dja@axtens.net> >>> --- >>> libstb/secvar/backend/edk2.h | 18 +++++++++--------- >>> 1 file changed, 9 insertions(+), 9 deletions(-) >>> >>> diff --git a/libstb/secvar/backend/edk2.h b/libstb/secvar/backend/edk2.h >>> index ef6d7c79e7ff..17df2b7bc536 100644 >>> --- a/libstb/secvar/backend/edk2.h >>> +++ b/libstb/secvar/backend/edk2.h >>> @@ -125,15 +125,15 @@ static const uuid_t EFI_CERT_RSA2048_GUID = {{ 0xe8, 0x66, 0x57, 0x3c, 0x9c, 0x2 >>> * TimeZone: -1440 to 1440 or 2047 >>> */ >>> struct efi_time { >>> - u16 year; >>> + le16 year; >>> u8 month; >>> u8 day; >>> u8 hour; >>> u8 minute; >>> u8 second; >>> u8 pad1; >>> - u32 nanosecond; >>> - s16 timezone; >>> + le32 nanosecond; >>> + s16 ENDIAN_TYPE timezone; >>> u8 daylight; >>> u8 pad2; >>> }; >> >> Why does it need to be annotated? None of the Linux tree's efi structs >> seem to have any endian annotations, is that just because they're only >> used on little endian platforms I wonder? > > I didn't check Linux's ones, that's interesting to know. The UEFI spec > explictly states that UEFI machines are little-endian machines (s1.8.1 > in v2.8), even when the underlying chip is capable of running in either > endianness, so that might explain it. Ah yes I guess it would, thanks. > > The rationale for changing them here is that if we don't change them, > every time we do > > u16 year = le16_to_cpu(time->year); > > we get a sparse warning saying that you can't do an endian conversion on > a non-endian-marked type. I suspect Linux EFI code doesn't bother with > the conversion and therefore we don't get the sparse warnings on Linux. Right, I'm asking about the timezone annotation though. Oh actually on re-reading what I wrote, that wasn't clear, there were two trains of thought colliding there. I'll try again, AFAIK you can use le16 for it and do `s16 tz = le16_to_cpu(efi->timezone)` and the signedness works out. Yes you do lose the signedness of the type when you do that but it seems to be the done thing. For non trivial cases you might have a host native structure as well and convert between them. > However, even if we were OK with skiboot being LE forever, we don't want > to get rid of the endianness conversions here, because this code is also > shared with secvarctl (and hopefully other things soon), and they can > run as BE and so do need to get endianness right. No the patch is good, my question was rhetorical as-in, why do we have to and not Linux, you answered that. Thanks, Nick
> I'll try again, AFAIK you can use le16 for it and do `s16 tz = > le16_to_cpu(efi->timezone)` and the signedness works out. > > Yes you do lose the signedness of the type when you do that but it seems > to be the done thing. For non trivial cases you might have a host native > structure as well and convert between them. > OK, I'm sold. I'll respin it with timezone just being le16. I think the comment above the data structure makes its signedness sufficiently clear. Kind regards, Daniel
diff --git a/libstb/secvar/backend/edk2.h b/libstb/secvar/backend/edk2.h index ef6d7c79e7ff..17df2b7bc536 100644 --- a/libstb/secvar/backend/edk2.h +++ b/libstb/secvar/backend/edk2.h @@ -125,15 +125,15 @@ static const uuid_t EFI_CERT_RSA2048_GUID = {{ 0xe8, 0x66, 0x57, 0x3c, 0x9c, 0x2 * TimeZone: -1440 to 1440 or 2047 */ struct efi_time { - u16 year; + le16 year; u8 month; u8 day; u8 hour; u8 minute; u8 second; u8 pad1; - u32 nanosecond; - s16 timezone; + le32 nanosecond; + s16 ENDIAN_TYPE timezone; u8 daylight; u8 pad2; }; @@ -163,15 +163,15 @@ typedef struct __packed { /// /// Total size of the signature list, including this header. /// - uint32_t SignatureListSize; + leint32_t SignatureListSize; /// /// Size of the signature header which precedes the array of signatures. /// - uint32_t SignatureHeaderSize; + leint32_t SignatureHeaderSize; /// /// Size of each signature. /// - uint32_t SignatureSize; + leint32_t SignatureSize; /// /// Header before the array of signatures. The format of this header is specified /// by the SignatureType. @@ -191,18 +191,18 @@ struct win_certificate { * The length of the entire certificate, including the length of the * header, in bytes. */ - u32 dw_length; + le32 dw_length; /* * The revision level of the WIN_CERTIFICATE structure. The current * revision level is 0x0200. */ - u16 w_revision; + le16 w_revision; /* * The certificate type. See WIN_CERT_TYPE_xxx for the UEFI certificate * types. The UEFI specification reserves the range of certificate type * values from 0x0EF0 to 0x0EFF. */ - u16 w_certificate_type; + le16 w_certificate_type; /* * The following is the actual certificate. The format of * the certificate depends on wCertificateType.
Recently we had an issue where we did the following: uint16_t year = le32_to_cpu(timestamp->year); This is wrong and will break on BE. However, we didn't catch this with sparse because there was a whole slew of warnings. The reason for the slew of warnings is that we didn't annotate the types that store little-endian specific data in edk2.h. Provide the appropriate annotations. We now get a single sparse warning for the file, which correctly identifies the issue: edk2-compat-process.c:374:46: warning: incorrect type in argument 1 (different base types) edk2-compat-process.c:374:46: expected restricted leint32_t [usertype] le_val edk2-compat-process.c:374:46: got restricted leint16_t const [usertype] year There's one annotation that I'm not super happy about - the annotation of efi_time->timezone. Happy for other ideas on that one. Signed-off-by: Daniel Axtens <dja@axtens.net> --- libstb/secvar/backend/edk2.h | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)