On 24 June 2018 at 16:57, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote: > On 24 June 2018 at 16:53, Lukas Wunner <lukas@xxxxxxxxx> wrote: >> On Sun, Jun 24, 2018 at 03:42:19PM +0200, Ard Biesheuvel wrote: >>> Considering we had other weird issues involving uint64_t types with >>> the TPM code just this week, > > Actually, that issue involved pointers to uint64_t types, so they seem > unrelated after all, but that still means the mixed mode code is > fragile in the presence of protocols taking uint64_t type arguments by > value. > >>> I wonder if this isn't a fundamental >>> problem with the mixed mode thunking, and so I need some help from the >>> x86 gurus (Ingo?) >> >> Looks like that's exactly what it does: >> >> /* >> * Convert x86-64 ABI params to i386 ABI >> */ >> subq $32, %rsp >> movl %esi, 0x0(%rsp) >> movl %edx, 0x4(%rsp) >> movl %ecx, 0x8(%rsp) >> movq %r8, %rsi >> movl %esi, 0xc(%rsp) >> movq %r9, %rsi >> movl %esi, 0x10(%rsp) >> >> I don't think there's a way to find out in thunk_64.S if the register >> was populated with a 64-bit or 32-bit value, but it *might* be possible >> to do it with a macro that accepts a __VA_ARGS__ argument, iterates >> over the parameters, checks whether a parameter is 64-bit and we're >> in mixed mode, and if so, passes in the high and low dword separately. >> > > That would be nice, but I don't see how the preprocessor can infer the > size of a type. As far as I can tell, only PciIo->Attributes () and efi_file_handle::open() [in efi_file_size()] are affected. I couldn't find any other occurrences of uint64_t arguments passed by value. For the latter case, the UEFI spec documents that the last u64 argument is ignored unless the one before it equals EFI_FILE_MODE_CREATE, which it doesn't. And I suppose we /could/ work around this occurrence by simply passing another couple of zero values just to be sure (and efi_file_size() is only called by the x86 code to handle initrd=, which I don't think anyone uses anyway?) [/me makes mental note to check whether we can deprecate initrd= handling in the stub for all architectures] So PciIo->Attributes () is really the only occurrence of this issue where we need to do something special. So I am leaning towards taking the easy way out here. and proposing some variant of Wilfried's original patch and simply pass two zeroes instead of one [almost as before] Thoughts? -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
