From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-x22c.google.com (mail-lf0-x22c.google.com [IPv6:2a00:1450:4010:c07::22c]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 86B961A1E31 for ; Thu, 20 Oct 2016 15:13:18 -0700 (PDT) Received: by mail-lf0-x22c.google.com with SMTP id b81so103207433lfe.1 for ; Thu, 20 Oct 2016 15:13:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:from:date:message-id:subject:to; bh=lQv3IAsxdVSYZn3ABEGOKYcBT4C4ndS7AYz8Bu1sZd0=; b=Hd/GGbjHUuYSTp0OohQ9dnU1YRWnYQH2uE6uP1xfdI3RkW13D3rywGRfo0sInb5hnU Za7qYnoqcIiKOLiqGsN0inb510wGhZDMLxyEMVyOTVQtBCqlSoEYdzchif8oAWoiLuI9 QmJVpWRsSdJvjgMjtbJPCNAo0S97HAmqlJfhHEEuGd1ozkaDasO12wJ5XWD+8vIMwTC4 jeDoMXwbPLE2Oz6J+Tt5hMUbabZa0rb7K+tJoT8PROlXFtQXxRHVZU2sZFdEvv8IlupU 8sHDV84Vi/se06B0XrU+BpYBRK94ARXauBfPWc7P/U06UovRwsYui8nSAMs/KiFSN2r+ H8Og== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:from:date:message-id:subject:to; bh=lQv3IAsxdVSYZn3ABEGOKYcBT4C4ndS7AYz8Bu1sZd0=; b=PyukW74aHHGxt/yG5ImkYV7yiWZesnjlML/qMN2B9RKpJaItQiROUXme8ezozD8g41 DRjYVlCdE9HhYL9L46c2C/e04sDh0pMPIbSVrcWfDLwTT9bKGpcl6Ed1o9Eh904RIOab jxtCjMJHB1WFng6B7D5lRhhImFTuY2eseIqM/bx9TFDditqSpBsBPWnDanmoyChcFqBV vVhqu2szgdg3HDUIAS/PetmXQ5hcfEFo7KgIuBAKvPDfXprrKg9iKdzdziFh4nUQKrHC LgPXoTUyAe+VsuttqCEZlcZs1E8JPSg41ACUILvsgehWcMEktfEMKd3ooq69PXqRkPhD V4yw== X-Gm-Message-State: AA6/9RmP5NGRkmGQyCCiydCh9KiMpOuXSwr1GepIlSTmMHdnC++ZnSr2hCTCfQGyOYZa/FqodQ7ZpJVPCsgydw== X-Received: by 10.25.141.3 with SMTP id p3mr2375254lfd.157.1477001594578; Thu, 20 Oct 2016 15:13:14 -0700 (PDT) MIME-Version: 1.0 Received: by 10.25.29.194 with HTTP; Thu, 20 Oct 2016 15:13:14 -0700 (PDT) From: valerij zaporogeci Date: Fri, 21 Oct 2016 01:13:14 +0300 Message-ID: To: edk2-devel Subject: PI Vol3, FvCheck pseudocode bugs X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 Oct 2016 22:13:19 -0000 Content-Type: text/plain; charset=UTF-8 Hi everyone. Reading the PI specification Volume 3 on FV stuff, I faced a lot of problems with figuring out what the pseudocode for Fv check should mean. Of course it's probably my fault, but still, the pseudocode has some obvious errata. Here I collected some: 1. Function FileCheck() states it "returns TRUE if the file is OK" But in the function FvCheck() we see: if(FileCheck(Fv, FilePtr) != 0){ FAILURE(); // inconsistent file system } 2. FileCheck() on the set flag EFI_FILE_CONSTRUCTION sets EFI_FILE_HEADER_INVALID and returns TRUE. The question is how next operations on that FV are going to locate the _next_ file header behind that with the HEADER_INVALID bit set? 3. FileCheck() on the set flag MARKED_FOR_UPDATE does check for DATA_VALID flag and if it is not set, returns FALSE. But the specification says a higher bit _overwrites_ meaning of lower ones. Why this check and what for? Could a file MARKED_FOR_UPDATE have DATA_VALID flag NOT set? 4. FileCheck() on the MARKED_FOR_UPDATE flag set for the case where the MARKED file _doesn't_ have a DATA_VALID duplicate file, whether clears MARKED bit directly or by copying the file. First question - what happens if there is no place to copy the file? And the most important - this situation of MARKED file and a non DATA_VALID duplicate happens on update failure, where the NEW file had no chance to get to the state DATA_VALID. This pseudocode suggests just to roll back the marking by means whether of copying or just clearing the bit. And then returns TRUE, meaning everything is fine. But is it? Such a check will result in half files being updated and half NOT, without any warning. It's hard to call it a "consistent" state. Maybe, here the function should signal that there is a need to continue an update from this stopped point of updating the MARKED file. NOT just rolling back the MARKED flag? 5. FileCheck() on the DELETED flag verifies both header checksum and file data checksum of the file. But the specification says that deleted file header is only valid and only "in as much as its length field is used in locating the next file in the firmware volume." So why checking the file data checksum? 6. Traversing the FV, FvCheck() relies on Exists() for stopping the traversal: for(FilePtr = FirstFile; Exists(Fv, FilePtr); FilePtr = NextFile(Fv, FilePtr)){ if(FileCheck (Fv, FilePtr) != 0){ FAILURE(); // inconsistent file system } } Exists() in turn relies on BufferErased(): BOOLEAN Exists(Fv, FilePtr){ return (BufferErased (Fv.ErasePolarity, FilePtr, sizeof (EFI_FIRMWARE_VOLUME_HEADER) == FALSE); } By the way, here we have parenthesis mismatch (copied above as is) Probably this was meant: BOOLEAN Exists(Fv, FilePtr){ return ( BufferErased(Fv.ErasePolarity, FilePtr, sizeof(EFI_FIRMWARE_VOLUME_HEADER)) == FALSE ); } As it is seen, BufferErased() is supposed to check whether the buffer has something written in it, or not. First, the check for every file is done with sizeof(EFI_FIRMWARE_VOLUME_HEADER) size parameter, which is not right I believe, EFI_FFS_FILE_HEADER or EFI_FFS_FILE_HEADER2 should be used instead. And the second, this checking for the end of traversal implies that in the end of the FV lies the area with all zeros or ones, depending on ERASE_POLARITY, thus a not programmed, free space. But why it should? And if it is not the case, this check is going to cause a constant check failures or even worse - different aborts, while FV is OK. I thought, for the purpose of finding the end of file traversal, there is concept of VTF. Isn't it for that? Wouldn't it be safer here to check for VTF file? Like: FilePtr = FirstFile; while(TRUE){ if(FileCheck (Fv, FilePtr) != 0){ FAILURE(); // inconsistent file system } if(IsFileVtf(FilePtr))break; FilePtr = NextFile(Fv, FilePtr); } And also, considering the property of Vtf of having its last byte as the last byte of FV, how this file could be updated at all? With this mechanism of marking for update and creating a duplicate? 7. There is no pseudocode for CheckFreeSpace() function. Nor description what it means and does. What a free space if we have VTF? Thank you.