Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #443 +/- ##
==========================================
- Coverage 34.22% 34.21% -0.01%
==========================================
Files 209 209
Lines 14087 14086 -1
==========================================
- Hits 4821 4820 -1
Misses 8377 8377
Partials 889 889
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
orangecms
force-pushed
the
me-fpt-fixes
branch
from
4bec2e6 to
6722304
Compare
pkg/uefi/meregion.go
Outdated
| func FindMEDescriptor(buf []byte) (int, error) { | ||
| if bytes.Equal(buf[16:16+len(MEFTPSignature)], MEFTPSignature) { | ||
| if bytes.Equal(buf[16:16+len(MEFPTSignature)], MEFPTSignature) { | ||
| // 16 + 4 since the descriptor starts after the signature |
There was a problem hiding this comment.
why not just call bytes.Index?
Also, so many people depend on this, it needs to come with a test at this point (you found a bug! bad! bad!) :-)
There was a problem hiding this comment.
I'm just extending the cases here. Since there is no public spec to my knowledge (I haven't found any) it's hard to say what would be correct. All this knowledge is based on RE.
There are cases where the assumption that the signature is either at the start or in the second 16 bytes holds, and me_cleaner does the same:
https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/refs/heads/stabilize-12239.46.B/util/me_cleaner/me_cleaner.py#579
Looking at more and newer images though, other cases show up, so MEAnalyzer tries a bunch of variants as well, with different implications:
https://github.com/platomav/MEAnalyzer/blob/6d898a6d3972ccec53099e05c5e84ec2abe79197/MEA.py#L11604
So yea bug or... case unknown up until now.
Would you just list the cases known so far for the test cases? I'll see what I can do.
There was a problem hiding this comment.
Test added. Tbh I don't see the real value, since I am now testing bytes.Index()... Would rather add some fixtures and use those instead. Then I'd be unsure because of ME binary distribution issues.
orangecms
force-pushed
the
me-fpt-fixes
branch
4 times, most recently
from
4ad1bbf to
8572ace
Compare
|
Removed the now needless dump of the first 20 bytes on error. The signature could be anywhere in the ME region. |
orangecms
force-pushed
the
me-fpt-fixes
branch
from
8572ace to
ca2d87a
Compare
|
... making the linter happy |
Signed-off-by: Daniel Maslowski <info@orangecms.org>
Easily confused: FPTR is the name of one partition, whereas FPT is the Firmware Partition Table (as far as we know). Signed-off-by: Daniel Maslowski <info@orangecms.org>
MEAnalyzer does the same thing. There are images where the FPT is not at the beginning of the ME region, e.g.: https://www.gigabyte.com/Motherboard/H410M-H-V3-rev-10-12/support#support-dl-bios Signed-off-by: Daniel Maslowski <info@orangecms.org>
…ound Signed-off-by: Daniel Maslowski <info@orangecms.org>
Signed-off-by: Daniel Maslowski <info@orangecms.org>
|
noticed that the only reference where information was taken from is now gone, so added the archive snapshot and more references |
orangecms
force-pushed
the
me-fpt-fixes
branch
from
ca2d87a to
c1ff9b1
Compare
| if r != test.res { | ||
| t.Errorf("got position %d want %d (%q)", r, test.res, e) | ||
| } | ||
| if test.res == -1 && e == nil { |
There was a problem hiding this comment.
you can put an err error member in the structure, and use errors.Is(err, test.err)
That would allow tests that succeed.
3 participants
No description provided.