diff mbox series

[v8,29/30] efi_loader: Pass address to fs_read()

Message ID 20180618140835.195901-30-sjg@chromium.org
State Superseded
Headers show
Series None | expand

Commit Message

Simon Glass June 18, 2018, 2:08 p.m. UTC
From: Alexander Graf <agraf@suse.de>

The fs_read() function wants to get an address rather than the
pointer to a buffer.

So let's convert the passed buffer from pointer back a the address
to make efi_loader on sandbox happier.

Signed-off-by: Alexander Graf <agraf@suse.de>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 lib/efi_loader/efi_file.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Alexander Graf June 18, 2018, 3:08 p.m. UTC | #1
On 06/18/2018 04:08 PM, Simon Glass wrote:
> From: Alexander Graf <agraf@suse.de>
>
> The fs_read() function wants to get an address rather than the
> pointer to a buffer.
>
> So let's convert the passed buffer from pointer back a the address
> to make efi_loader on sandbox happier.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> Changes in v8: None
> Changes in v7: None
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>   lib/efi_loader/efi_file.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> index e6a15bcb52..2107730ba5 100644
> --- a/lib/efi_loader/efi_file.c
> +++ b/lib/efi_loader/efi_file.c
> @@ -9,6 +9,7 @@
>   #include <charset.h>
>   #include <efi_loader.h>
>   #include <malloc.h>
> +#include <mapmem.h>
>   #include <fs.h>
>   
>   /* GUID for file system information */
> @@ -232,8 +233,10 @@ static efi_status_t file_read(struct file_handle *fh, u64 *buffer_size,
>   		void *buffer)
>   {
>   	loff_t actread;
> +	/* fs_read expects buffer address, not pointer */
> +	uintptr_t buffer_addr = (uintptr_t)map_to_sysmem(buffer);

As you've seen with your patch on the stack based map_to_sysmem() 
calculation, map_to_sysmem() really should only be used on pointers that 
we are sure come from RAM. With EFI binaries, that isn't true because of 
the stack, but it might be true due to other reasons too on real 
hardware, such as direct read/write to/from flash.

I think it's much safer to stay in pointer land once we passed the addr 
-> ptr boundary. Going from ptr -> addr is usually a recipe for disaster.


Alex
Simon Glass June 21, 2018, 2:01 a.m. UTC | #2
Hi Alex,

On 18 June 2018 at 09:08, Alexander Graf <agraf@suse.de> wrote:
> On 06/18/2018 04:08 PM, Simon Glass wrote:
>>
>> From: Alexander Graf <agraf@suse.de>
>>
>> The fs_read() function wants to get an address rather than the
>> pointer to a buffer.
>>
>> So let's convert the passed buffer from pointer back a the address
>> to make efi_loader on sandbox happier.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v8: None
>> Changes in v7: None
>> Changes in v6: None
>> Changes in v5: None
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>>
>>   lib/efi_loader/efi_file.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
>> index e6a15bcb52..2107730ba5 100644
>> --- a/lib/efi_loader/efi_file.c
>> +++ b/lib/efi_loader/efi_file.c
>> @@ -9,6 +9,7 @@
>>   #include <charset.h>
>>   #include <efi_loader.h>
>>   #include <malloc.h>
>> +#include <mapmem.h>
>>   #include <fs.h>
>>     /* GUID for file system information */
>> @@ -232,8 +233,10 @@ static efi_status_t file_read(struct file_handle *fh,
>> u64 *buffer_size,
>>                 void *buffer)
>>   {
>>         loff_t actread;
>> +       /* fs_read expects buffer address, not pointer */
>> +       uintptr_t buffer_addr = (uintptr_t)map_to_sysmem(buffer);
>
>
> As you've seen with your patch on the stack based map_to_sysmem()
> calculation, map_to_sysmem() really should only be used on pointers that we
> are sure come from RAM. With EFI binaries, that isn't true because of the
> stack, but it might be true due to other reasons too on real hardware, such
> as direct read/write to/from flash.
>
> I think it's much safer to stay in pointer land once we passed the addr ->
> ptr boundary. Going from ptr -> addr is usually a recipe for disaster.

We actually have no choice but to support this. As mentioned elsewhere
addresses are the main currency in U-Boot and we should not look to
convert it to use pointers. They are much less enjoyable to work with.
The above patch is pretty simple.

Regards,
Simon
diff mbox series

Patch

diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
index e6a15bcb52..2107730ba5 100644
--- a/lib/efi_loader/efi_file.c
+++ b/lib/efi_loader/efi_file.c
@@ -9,6 +9,7 @@ 
 #include <charset.h>
 #include <efi_loader.h>
 #include <malloc.h>
+#include <mapmem.h>
 #include <fs.h>
 
 /* GUID for file system information */
@@ -232,8 +233,10 @@  static efi_status_t file_read(struct file_handle *fh, u64 *buffer_size,
 		void *buffer)
 {
 	loff_t actread;
+	/* fs_read expects buffer address, not pointer */
+	uintptr_t buffer_addr = (uintptr_t)map_to_sysmem(buffer);
 
-	if (fs_read(fh->path, (ulong)buffer, fh->offset,
+	if (fs_read(fh->path, buffer_addr, fh->offset,
 		    *buffer_size, &actread))
 		return EFI_DEVICE_ERROR;