[edk2-devel] [edk2 PATCH 42/48] OvmfPkg/VirtioFsDxe: add helper for composing rename/move destination path
Laszlo Ersek
lersek at redhat.com
Sat Dec 19 22:40:27 UTC 2020
On 12/18/20 18:39, Ard Biesheuvel wrote:
> On 12/16/20 10:11 PM, Laszlo Ersek wrote:
>> The EFI_FILE_PROTOCOL.SetInfo() member is somewhat under-specified; one of
>> its modes of operation is renaming/moving the file.
>>
>> In order to create the destination pathname in canonical format, 2*2=4
>> cases have to be considered. For the sake of discussion, assume the
>> current canonical pathname of a VIRTIO_FS_FILE is "/home/user/f1.txt".
>> Then, consider the following rename/move requests from
>> EFI_FILE_PROTOCOL.SetInfo():
>>
>> Destination requested Destination Move into Destination in
>> by SetInfo() relative? directory? canonical format
>> --------------------- ----------- ---------- -----------------------
>> L"\\dir\\f2.txt" no no "/dir/f2.txt"
>
> What happens in the above case if /dir/f2.txt is an existing directory?
Short answer:
The present patch only constructs the destination pathname. The rename
attempt you describe is caught
- either by the subsequent patch, if the existing dest directory is open
by the guest driver,
- or else, by the host kernel, due to the RENAME_NOREPLACE flag set in
the patch before this one.
Long (very long) answer, in opposite order of the above cases:
- If "/dir/f2.txt" were an existing name (regardless of the type of the
host-side inode that it referred to), then the FUSE_RENAME2 request
would fail: the host kernel would reject the renameat2() system call
made by virtiofsd. This would be due to the RENAME_NOREPLACE flag:
https://man7.org/linux/man-pages/man2/rename.2.html
include/uapi/linux/fs.h
which is set in
[edk2 PATCH 41/48]
OvmfPkg/VirtioFsDxe: implement the wrapper function for FUSE_RENAME2
using the macro name VIRTIO_FS_FUSE_RENAME2_REQ_F_NOREPLACE.
Thus, if the request reached the host kernel, an -EEXIST errno would
come back to the guest driver.
(
- If the movement source were a non-directory and the destination were a
directory, then that would fail (also in the host kernel at the
latest) even with the simpler (flag-less) FUSE_RENAME request, which
virtiofsd translates to the renameat() syscall, with -EISDIR.
- If both source and destination were directories, and the destination
were not empty, then even the flag-less renameat() would fail with
-ENOTEMPTY.
- If both source and destination were directories, and the destination
were empty, then renameat() would replace the destination [*]; but
renameat2() with RENAME_NOREPLACE will not.
[*] mkdir source-dir target-dir
ls -lid source-dir target-dir
touch source-dir/file.txt
mv -T source-dir target-dir
ls -lid target-dir
ls -l target-dir/file.txt
)
Then, in addition to RENAME_NOREPLACE, there's a guest-side check in
[edk2 PATCH 43/48]
OvmfPkg/VirtioFsDxe: handle file rename/move in EFI_FILE_PROTOCOL.SetInfo
that was inspired by "FatPkg/EnhancedFatDxe". Namely:
> + //
> + // Check if the rename would break the canonical pathnames of other
> + // VIRTIO_FS_FILE instances of the same VIRTIO_FS.
> + //
> + if (VirtioFsFile->IsDirectory) {
> + UINTN PathLen;
> + LIST_ENTRY *OpenFilesEntry;
> +
> + PathLen = AsciiStrLen (VirtioFsFile->CanonicalPathname);
> + BASE_LIST_FOR_EACH (OpenFilesEntry, &VirtioFs->OpenFiles) {
> + VIRTIO_FS_FILE *OtherFile;
> +
> + OtherFile = VIRTIO_FS_FILE_FROM_OPEN_FILES_ENTRY (OpenFilesEntry);
> + if (OtherFile != VirtioFsFile &&
> + AsciiStrnCmp (VirtioFsFile->CanonicalPathname,
> + OtherFile->CanonicalPathname, PathLen) == 0 &&
> + (OtherFile->CanonicalPathname[PathLen] == '\0' ||
> + OtherFile->CanonicalPathname[PathLen] == '/')) {
> + //
> + // OtherFile refers to the same directory as VirtioFsFile, or is a
> + // (possibly indirect) child of the directory referred to by
> + // VirtioFsFile.
> + //
> + Status = EFI_ACCESS_DENIED;
> + goto FreeDestination;
> + }
> + }
> + }
This is why "VIRTIO_FS.OpenFiles" is a list, and not just a reference
count -- for this simple guest-side check, I needed to go through the
other open VIRTIO_FS_FILEs for the same VIRTIO_FS one by one. Just
knowing how many of them existed wouldn't be enough.
This guest-side check is by no means foolproof; after all, you can do
whatever you want on the host side, underneath the guest driver's feet.
But, catching such "async tricks" is not a goal for this driver.
EFI_FILE_PROTOCOL is not equipped to deal with such async changes by
design. At best, it can return EFI_MEDIA_CHANGED, when (e.g.) removable
media is replaced. But even if I could detect such a situation in the
virtio-fs driver, it would be counter-productive: when a host-side file
changes, that's something the guest wants to pick up one way or another,
we don't want the driver to switch to returning EFI_MEDIA_CHANGED for
all further requests indiscriminately.
Synchronization between host and guest is pretty simple for the
interactive use case: whenever your shell prompt returns, on the host or
in the guest (meaning the UEFI shell prompt in the latter), you can
Alt-TAB to the other terminal, and manipulate files from there.
Synchronization between host-side and guest-side scripts should be
possible with polling directory listings, and renaming / moving regular
files (files should be prepared in "private" directories, and then moved
into place when done, for the other side to notice).
So, the above guest-side check exists for the usual case when the
relevant sub-hierarchy of the shared directory doesn't change
asynchronously to VIRTIO_FS_FILE's being open; when the guest-side check
fires in this optimistic situation, the FUSE_RENAME2 request isn't even
sent. And for when the guest-side check isn't good enough, that's when
the RENAME_NOREPLACE flag becomes relevant -- I wanted to avoid
unwittingly deleting entries in the shared directory by guest-initiated
renames.
Sorry about the long answer. I feel it's not really possible to address
your question without talking about asynchrony between host and guest. I
had thought about it before, and I figured, shoehorning a shared
filesystem into a non-shared filesystem abstraction should be acceptable
this way. The use case is to support Alt-TABbing between your guest and
host terminals, and running such UEFI unit tests in the guest that take
input from the host filesystem and produce output to the host
filesystem. A highly async / parallel operation is a non-goal.
Thanks!
Laszlo
>
>> L"\\dir\\" no yes "/dir/f1.txt"
>> L"dir\\f2.txt" yes no "/home/user/dir/f2.txt"
>> L"dir\\" yes yes "/home/user/dir/f1.txt"
>>
>> Add the VirtioFsComposeRenameDestination() function, for composing the
>> last column from the current canonical pathname and the SetInfo() input.
>>
>> The function works on the following principles:
>>
>> - The prefix of the destination path is "/", if the SetInfo() rename
>> request is absolute.
>>
>> Otherwise, the dest prefix is the "current directory" (the most specific
>> parent directory) of the original pathname (in the above example,
>> "/home/user").
>>
>> - The suffix of the destination path is precisely the SetInfo() request
>> string, if the "move into directory" convenience format -- the trailing
>> backslash -- is not used. (In the above example, L"\\dir\\f2.txt" and
>> L"dir\\f2.txt".)
>>
>> Otherwise, the suffix is the SetInfo() request, plus the original
>> basename (in the above example, L"\\dir\\f1.txt" and L"dir\\f1.txt").
>>
>> - The complete destination is created by fusing the dest prefix and the
>> dest suffix, using the VirtioFsAppendPath() function.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel at arm.com>
>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>> Cc: Philippe Mathieu-Daudé <philmd at redhat.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3097
>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>> ---
>> OvmfPkg/VirtioFsDxe/VirtioFsDxe.h | 8 +
>> OvmfPkg/VirtioFsDxe/Helpers.c | 194 ++++++++++++++++++++
>> 2 files changed, 202 insertions(+)
>>
>> diff --git a/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h b/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h
>> index 9334e5434c51..a6dfac71f4a7 100644
>> --- a/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h
>> +++ b/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h
>> @@ -257,16 +257,24 @@ VirtioFsLookupMostSpecificParentDir (
>>
>> EFI_STATUS
>> VirtioFsGetBasename (
>> IN CHAR8 *Path,
>> OUT CHAR16 *Basename OPTIONAL,
>> IN OUT UINTN *BasenameSize
>> );
>>
>> +EFI_STATUS
>> +VirtioFsComposeRenameDestination (
>> + IN CHAR8 *LhsPath8,
>> + IN CHAR16 *RhsPath16,
>> + OUT CHAR8 **ResultPath8,
>> + OUT BOOLEAN *RootEscape
>> + );
>> +
>> EFI_STATUS
>> VirtioFsFuseAttrToEfiFileInfo (
>> IN VIRTIO_FS_FUSE_ATTRIBUTES_RESPONSE *FuseAttr,
>> OUT EFI_FILE_INFO *FileInfo
>> );
>>
>> EFI_STATUS
>> VirtioFsFuseDirentPlusToEfiFileInfo (
>> diff --git a/OvmfPkg/VirtioFsDxe/Helpers.c b/OvmfPkg/VirtioFsDxe/Helpers.c
>> index cdaa8557a17b..b741cf753495 100644
>> --- a/OvmfPkg/VirtioFsDxe/Helpers.c
>> +++ b/OvmfPkg/VirtioFsDxe/Helpers.c
>> @@ -1778,16 +1778,210 @@ VirtioFsGetBasename (
>> }
>>
>> for (Idx = LastComponent; Idx < PathSize; Idx++) {
>> Basename[Idx - LastComponent] = Path[Idx];
>> }
>> return EFI_SUCCESS;
>> }
>>
>> +/**
>> + Format the destination of a rename/move operation as a dynamically allocated
>> + canonical pathname.
>> +
>> + Any dot-dot in RhsPath16 that would remove the root directory is dropped, and
>> + reported through RootEscape, without failing the function call.
>> +
>> + @param[in] LhsPath8 The source pathname operand of the rename/move
>> + operation, expressed as a canonical pathname (as
>> + defined in the description of VirtioFsAppendPath()).
>> + The root directory "/" cannot be renamed/moved, and
>> + will be rejected.
>> +
>> + @param[in] RhsPath16 The destination pathname operand expressed as a
>> + UEFI-style CHAR16 pathname.
>> +
>> + If RhsPath16 starts with a backslash, then RhsPath16
>> + is considered absolute. Otherwise, RhsPath16 is
>> + interpreted relative to the most specific parent
>> + directory found in LhsPath8.
>> +
>> + Independently, if RhsPath16 ends with a backslash
>> + (i.e., RhsPath16 is given in the "move into
>> + directory" convenience form), then RhsPath16 is
>> + interpreted with the basename of LhsPath8 appended.
>> + Otherwise, the last pathname component of RhsPath16
>> + is taken as the last pathname component of the
>> + rename/move destination.
>> +
>> + An empty RhsPath16 is rejected.
>> +
>> + @param[out] ResultPath8 The POSIX-style, canonical format pathname that
>> + leads to the renamed/moved file. After use, the
>> + caller is responsible for freeing ResultPath8.
>> +
>> + @param[out] RootEscape Set to TRUE if at least one dot-dot component in
>> + RhsPath16 attempted to escape the root directory;
>> + set to FALSE otherwise.
>> +
>> + @retval EFI_SUCCESS ResultPath8 has been produced. RootEscape has
>> + been output.
>> +
>> + @retval EFI_INVALID_PARAMETER LhsPath8 is "/".
>> +
>> + @retval EFI_INVALID_PARAMETER RhsPath16 is zero-length.
>> +
>> + @retval EFI_INVALID_PARAMETER RhsPath16 failed the
>> + VIRTIO_FS_MAX_PATHNAME_LENGTH check.
>> +
>> + @retval EFI_OUT_OF_RESOURCES Memory allocation failed.
>> +
>> + @retval EFI_OUT_OF_RESOURCES ResultPath8 would have failed the
>> + VIRTIO_FS_MAX_PATHNAME_LENGTH check.
>> +
>> + @retval EFI_UNSUPPORTED RhsPath16 contains a character that either
>> + falls outside of the printable ASCII set, or
>> + is a forward slash.
>> +**/
>> +EFI_STATUS
>> +VirtioFsComposeRenameDestination (
>> + IN CHAR8 *LhsPath8,
>> + IN CHAR16 *RhsPath16,
>> + OUT CHAR8 **ResultPath8,
>> + OUT BOOLEAN *RootEscape
>> + )
>> +{
>> + //
>> + // Lengths are expressed as numbers of characters (CHAR8 or CHAR16),
>> + // excluding terminating NULs. Sizes are expressed as byte counts, including
>> + // the bytes taken up by terminating NULs.
>> + //
>> + UINTN RhsLen;
>> + UINTN LhsBasename16Size;
>> + EFI_STATUS Status;
>> + UINTN LhsBasenameLen;
>> + UINTN DestSuffix16Size;
>> + CHAR16 *DestSuffix16;
>> + CHAR8 *DestPrefix8;
>> +
>> + //
>> + // An empty destination operand for the rename/move operation is not allowed.
>> + //
>> + RhsLen = StrLen (RhsPath16);
>> + if (RhsLen == 0) {
>> + return EFI_INVALID_PARAMETER;
>> + }
>> + //
>> + // Enforce length restriction on RhsPath16.
>> + //
>> + if (RhsLen > VIRTIO_FS_MAX_PATHNAME_LENGTH) {
>> + return EFI_INVALID_PARAMETER;
>> + }
>> +
>> + //
>> + // Determine the length of the basename of LhsPath8.
>> + //
>> + LhsBasename16Size = 0;
>> + Status = VirtioFsGetBasename (LhsPath8, NULL, &LhsBasename16Size);
>> + ASSERT (Status == EFI_BUFFER_TOO_SMALL);
>> + ASSERT (LhsBasename16Size >= sizeof (CHAR16));
>> + ASSERT (LhsBasename16Size % sizeof (CHAR16) == 0);
>> + LhsBasenameLen = LhsBasename16Size / sizeof (CHAR16) - 1;
>> + if (LhsBasenameLen == 0) {
>> + //
>> + // The root directory cannot be renamed/moved.
>> + //
>> + return EFI_INVALID_PARAMETER;
>> + }
>> +
>> + //
>> + // Resolve the "move into directory" convenience form in RhsPath16.
>> + //
>> + if (RhsPath16[RhsLen - 1] == L'\\') {
>> + //
>> + // Append the basename of LhsPath8 as a CHAR16 string to RhsPath16.
>> + //
>> + DestSuffix16Size = RhsLen * sizeof (CHAR16) + LhsBasename16Size;
>> + DestSuffix16 = AllocatePool (DestSuffix16Size);
>> + if (DestSuffix16 == NULL) {
>> + return EFI_OUT_OF_RESOURCES;
>> + }
>> + CopyMem (DestSuffix16, RhsPath16, RhsLen * sizeof (CHAR16));
>> + Status = VirtioFsGetBasename (LhsPath8, DestSuffix16 + RhsLen,
>> + &LhsBasename16Size);
>> + ASSERT_EFI_ERROR (Status);
>> + } else {
>> + //
>> + // Just create a copy of RhsPath16.
>> + //
>> + DestSuffix16Size = (RhsLen + 1) * sizeof (CHAR16);
>> + DestSuffix16 = AllocateCopyPool (DestSuffix16Size, RhsPath16);
>> + if (DestSuffix16 == NULL) {
>> + return EFI_OUT_OF_RESOURCES;
>> + }
>> + }
>> +
>> + //
>> + // If the destination operand is absolute, it will be interpreted relative to
>> + // the root directory.
>> + //
>> + // Otherwise (i.e., if the destination operand is relative), then create the
>> + // canonical pathname that the destination operand is interpreted relatively
>> + // to; that is, the canonical pathname of the most specific parent directory
>> + // found in LhsPath8.
>> + //
>> + if (DestSuffix16[0] == L'\\') {
>> + DestPrefix8 = AllocateCopyPool (sizeof "/", "/");
>> + if (DestPrefix8 == NULL) {
>> + Status = EFI_OUT_OF_RESOURCES;
>> + goto FreeDestSuffix16;
>> + }
>> + } else {
>> + UINTN LhsLen;
>> + UINTN DestPrefixLen;
>> +
>> + //
>> + // Strip the basename of LhsPath8.
>> + //
>> + LhsLen = AsciiStrLen (LhsPath8);
>> + ASSERT (LhsBasenameLen < LhsLen);
>> + DestPrefixLen = LhsLen - LhsBasenameLen;
>> + ASSERT (LhsPath8[DestPrefixLen - 1] == '/');
>> + //
>> + // If we're not at the root directory, strip the slash too.
>> + //
>> + if (DestPrefixLen > 1) {
>> + DestPrefixLen--;
>> + }
>> + DestPrefix8 = AllocatePool (DestPrefixLen + 1);
>> + if (DestPrefix8 == NULL) {
>> + Status = EFI_OUT_OF_RESOURCES;
>> + goto FreeDestSuffix16;
>> + }
>> + CopyMem (DestPrefix8, LhsPath8, DestPrefixLen);
>> + DestPrefix8[DestPrefixLen] = '\0';
>> + }
>> +
>> + //
>> + // Now combine DestPrefix8 and DestSuffix16 into the final canonical
>> + // pathname.
>> + //
>> + Status = VirtioFsAppendPath (DestPrefix8, DestSuffix16, ResultPath8,
>> + RootEscape);
>> +
>> + FreePool (DestPrefix8);
>> + //
>> + // Fall through.
>> + //
>> +FreeDestSuffix16:
>> + FreePool (DestSuffix16);
>> +
>> + return Status;
>> +}
>> +
>> /**
>> Convert select fields of a VIRTIO_FS_FUSE_ATTRIBUTES_RESPONSE object to
>> corresponding fields in EFI_FILE_INFO.
>>
>> @param[in] FuseAttr The VIRTIO_FS_FUSE_ATTRIBUTES_RESPONSE object to
>> convert the relevant fields from.
>>
>> @param[out] FileInfo The EFI_FILE_INFO structure to modify. Importantly, the
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69258): https://edk2.groups.io/g/devel/message/69258
Mute This Topic: https://groups.io/mt/79024445/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list