| Message ID | 20180618140835.195901-30-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | efi: Enable sandbox support for EFI loader | expand |
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
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 --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;