-
-
Notifications
You must be signed in to change notification settings - Fork 27
Properly perform checksums on partition table arrays when partition names contains garbage data after the null terminator #142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
5362d64 to
c14da28
Compare
|
Hello and thank you so much for your contribution 🙏 I like it thanks! But I think you got the wrong approach on fixing this issue. The partition name is store in a public struct called What do you think about it? /// A wrapper type for `String` that represents a partition's name.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct PartitionName(String); // <-- 2 fields, one for the parsed string, one for the original bytes
impl PartitionName {
/// Extracts a string slice containing the entire `PartitionName`.
pub fn as_str(&self) -> &str {
self.0.as_str() // <-- keep returning the parsed string normally
}
}
impl From<&str> for PartitionName {
fn from(value: &str) -> PartitionName {
PartitionName(value.to_string()) // <-- also generate the field for the UTF-16 bytes
}
}
impl Serialize for PartitionName {
fn serialize<S>(&self, serializer: S) -> std::result::Result<S::Ok, S::Error>
where
S: Serializer,
{
// ... serialize the byte fields instead of the string
}
} |
Yes, sure! I thought it probably would be better for it to store a copy of the raw bytes so it keeps exactly what it is. Now that you are okay with just leveraging PartitionName (and since dumped contents are actually packed), I'll try to do what you suggested! |
c14da28 to
0d6ef41
Compare
|
Hi @cecton, I've updated the code according your suggestion. It takes a rather dirty (I think) approach to deserialize UTF-16 characters. |
52b067d to
98a4a20
Compare
|
@cecton PR has been updated. Thanks! |
cecton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last bit sorry 🙏
It's okay. I'll update it tomorrow. |
98a4a20 to
267cf40
Compare
Some partitioning tools like ones based on libparted (parted and GParted) don't wipe the entire GPT partition name field before writing new partition names. Additionally, it is not guaranteed to be properly zeroed since the UEFI specification does not clearly state that. Use a dedicated raw buffer holding the entire partition name field without any conversion for performing checksums and serializations. Signed-off-by: Xinhui Yang <[email protected]>
disk3.img contains a label written with 512-byte sector size. disk4.img contains a label with 4096-byte sector size. This test makes sure the CRC checksum performs correctly when the partition table contains such name fields. Both labels contains a partition with (intended) trailing garbage in its name: 0000: af 3d c6 0f 83 84 72 47 8e 79 3d 69 d8 47 7d e4 .=....rG .y=i.G}. 0010: 84 d6 89 47 76 ba 4e 45 b2 50 e0 65 2a 5a 4f 76 ...Gv.NE .P.e*ZOv 0020: 32 00 00 00 00 00 00 00 41 00 00 00 00 00 00 00 2....... A....... 0030: 00 00 00 00 00 00 00 00 4e 00 61 00 6d 00 65 00 ........ N.a.m.e. 0040: 20 00 77 00 69 00 74 00 68 00 20 00 67 00 61 00 .w.i.t. h. .g.a. 0050: 72 00 62 00 61 00 67 00 65 00 00 00 2d 00 3e 00 r.b.a.g. e...-.>. 0060: 20 00 67 00 61 00 72 00 62 00 61 00 67 00 65 00 .g.a.r. b.a.g.e. 0070: 20 00 68 00 65 00 72 00 65 00 21 00 00 00 00 00 .h.e.r. e.!.....
267cf40 to
0eb054f
Compare
Some partitioning tools like ones based on libparted (parted and GParted) don't wipe the entire GPT partition name field before writing new partition names. Additionally, it is not guaranteed to be properly zeroed since the UEFI specification does not clearly state that.
Current checksuming logic uses processed partition names which ignores garbage data after the null terminator, and reports an CRC error if such partition table is encountered:
(This test program uses
gptmanto scan for ESP partition(s).)You can easily recreate this issue using either parted or GParted, first name a partition with a longer name, then rename it with a shorter name. You can see the remining bytes are not zeroed:
Use a dedicated raw buffer which directly read or write to the disk to perform checksums without explicit string handling.