RE: [PATCH 2/2] drivers: firmware: efi: install new fdt in configuration table

2018-04-29 Thread Pankaj Bansal 


> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Thursday, April 26, 2018 2:12 PM
> To: Grant Likely 
> Cc: Udit Kumar ; Leif Lindholm
> ; Pankaj Bansal ;
> mark.rutl...@arm.com; linux-efi@vger.kernel.org; Varun Sethi
> ; Wasim Khan ; n...@arm.com; Rod
> Dorris 
> Subject: Re: [PATCH 2/2] drivers: firmware: efi: install new fdt in 
> configuration
> table
> 
> On 26 April 2018 at 10:37, Grant Likely  wrote:
> > On 25/04/2018 15:48, Udit Kumar wrote:
> >>
> >>
> >>
> >>> -Original Message-
> >>> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> >>> Sent: Wednesday, April 25, 2018 4:20 PM
> >>> To: Grant Likely 
> >>> Cc: Udit Kumar ; Ard Biesheuvel
> >>> ; Pankaj Bansal ;
> >>> mark.rutl...@arm.com; linux-efi@vger.kernel.org; Varun Sethi
> >>> ; Wasim Khan ; n...@arm.com;
> Rod
> >>> Dorris 
> >>> Subject: Re: [PATCH 2/2] drivers: firmware: efi: install new fdt in
> >>> configuration table
> >>>
> >>> On Wed, Apr 25, 2018 at 11:04:13AM +0100, Grant Likely wrote:
> 
>  On 25/04/2018 07:49, Udit Kumar wrote:
> >>
> >> 2) Kernel provides/overrides DTB
> >> The kernel always has the option of loading it's own DTB, either
> >> because firmware doesn't provide one, or it needs a newer DTB.
> >> This is similar to CONFIG_ACPI_CUSTOM_DSDT in ACPI land. However,
> >> if the kernel is overriding the DTB, then firmware has no part of
> >> it, and it should not be allowed to modify the DTB at
> >> ExitBootServices()
> >>>
> >>> time.
> >
> >
> > For our platforms at least, this will not work unless firmware is
> > doing modifications.  I think this I likely true for Ard as well.
> > Firmware has a logic to modify default DTB for available devices,
> > reading board configuration programming required muxes, based upon
> > this removing/adding devices into DTB.
> > IMO, this will not be an optimal way to use untouched DTB provided
> > by
> >>>
> >>> kernel.
> 
> 
>  Right, which is part of the reason for insisting on the firmare
>  being responsible. If the kernel or Grub overrides the DTB, then it
>  is an explicit
>  *rejection* of anything firmware provides; presumable because
>  something is broken and it has to be worked around.
> >>>
> >>>
> >>> So, the functionality in GRUB to replace a firmware-provided
> >>> devicetree needs to go. And dtb= loading should ideally be made
> >>> dependent on some sort of DEBUG config option.
> >>
> >>
> >> In my view , dtb= sort of option should go away. If we are booting
> >> some debug image in development environment then why not to re-flash
> >> DTB
> >
> >
> > I don't want to drop the prototyping use cases on the floor. ie.
> > Wiring up a development board to external hardware. The development
> > board is already supported, but the user is going to be experimenting
> > with DT changes as they go. Reflashing the DT is not a good option in
> > this case. It is more usefule to be able to load the DT modifications
> > at runtime before booting the kernel, because they will change every boot.
> >
> > I can see this scenario playing out for both full DT replacement and
> > DTBO scenarios.
> >
> 
> This applies mainly to prototyping under the OS, right?
> 
> One aspect we did not consider yet I think is the use of DT to describe the
> platform to ARM-TF and UEFI itself. I expect this to become more common
> going forward, and will make replacing just the DTB even trickier.

Should we keep the dtb for UEFI and OS separate or not? The bootloader is 
expected to 
control only those devices from which it expects to get the OS to boot. The dtb 
file
for bootloader will contain only these devices' nodes, while the dtb passed to 
OS will contain
list of all devices that are available on a platform.

> 
> So overriding the DTB like we allow for DSDT makes perfect sense, but as a
> development feature only. I still think the idea of just dropping an updated 
> DTB
> into an existing firmware build consisting of ARM-TF, UEFI etc (and perhaps 
> even
> OP-TEE?) is not something that is going to be feasible in general.
N�r��yb�X��ǧv�^�)޺{.n�+{�y^n�r���z���h�&���G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�

[PATCH v4 2/4] efi: align efi_pci_io_protocol typedefs to type naming convention

2018-04-29 Thread Ard Biesheuvel 
In order to use the helper macros that perform type mangling with the
EFI PCI I/O protocol struct typedefs, align their Linux typenames with
the convention we use for definitionns that originate in the UEFI spec,
and add the trailing _t to each.

Signed-off-by: Ard Biesheuvel 
---
 arch/x86/boot/compressed/eboot.c | 8 
 include/linux/efi.h  | 6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 09f36c0d9d4f..3994f48c4043 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -109,7 +109,7 @@ void efi_char16_printk(efi_system_table_t *table, 
efi_char16_t *str)
 }
 
 static efi_status_t
-__setup_efi_pci32(efi_pci_io_protocol_32 *pci, struct pci_setup_rom **__rom)
+__setup_efi_pci32(efi_pci_io_protocol_32_t *pci, struct pci_setup_rom **__rom)
 {
struct pci_setup_rom *rom = NULL;
efi_status_t status;
@@ -176,7 +176,7 @@ static void
 setup_efi_pci32(struct boot_params *params, void **pci_handle,
unsigned long size)
 {
-   efi_pci_io_protocol_32 *pci = NULL;
+   efi_pci_io_protocol_32_t *pci = NULL;
efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
u32 *handles = (u32 *)(unsigned long)pci_handle;
efi_status_t status;
@@ -218,7 +218,7 @@ setup_efi_pci32(struct boot_params *params, void 
**pci_handle,
 }
 
 static efi_status_t
-__setup_efi_pci64(efi_pci_io_protocol_64 *pci, struct pci_setup_rom **__rom)
+__setup_efi_pci64(efi_pci_io_protocol_64_t *pci, struct pci_setup_rom **__rom)
 {
struct pci_setup_rom *rom;
efi_status_t status;
@@ -284,7 +284,7 @@ static void
 setup_efi_pci64(struct boot_params *params, void **pci_handle,
unsigned long size)
 {
-   efi_pci_io_protocol_64 *pci = NULL;
+   efi_pci_io_protocol_64_t *pci = NULL;
efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
u64 *handles = (u64 *)(unsigned long)pci_handle;
efi_status_t status;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 3016d8c456bc..56add823f190 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -397,7 +397,7 @@ typedef struct {
u32 set_bar_attributes;
u64 romsize;
u32 romimage;
-} efi_pci_io_protocol_32;
+} efi_pci_io_protocol_32_t;
 
 typedef struct {
u64 poll_mem;
@@ -417,7 +417,7 @@ typedef struct {
u64 set_bar_attributes;
u64 romsize;
u64 romimage;
-} efi_pci_io_protocol_64;
+} efi_pci_io_protocol_64_t;
 
 typedef struct {
void *poll_mem;
@@ -437,7 +437,7 @@ typedef struct {
void *set_bar_attributes;
uint64_t romsize;
void *romimage;
-} efi_pci_io_protocol;
+} efi_pci_io_protocol_t;
 
 #define EFI_PCI_IO_ATTRIBUTE_ISA_MOTHERBOARD_IO 0x0001
 #define EFI_PCI_IO_ATTRIBUTE_ISA_IO 0x0002
-- 
2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 4/4] efi/x86: Ignore unrealistically large option roms

2018-04-29 Thread Ard Biesheuvel 
From: Hans de Goede 

setup_efi_pci() tries to save a copy of each PCI option ROM as this may
be necessary for the device driver for the PCI device to have access too.

On some systems the efi_pci_io_protocol's romimage and romsize fields
contain invalid data, which looks a bit like pointers pointing back into
other EFI code or data. Interpreting these pointers as romsize leads to
a very large value and if we then try to alloc this amount of memory to
save a copy the alloc call fails.

This leads to a "Failed to alloc mem for rom" error being printed on the
EFI console for each PCI device.

This commit avoids the printing of these errors, by checking romsize before
doing the alloc and if it is larger then the EFI spec limit of 16 MiB
silently ignore the ROM fields instead of trying to alloc mem and fail.

Signed-off-by: Hans de Goede 
[ardb: deduplicate 32/64 bit changes, use SZ_16M symbolic constant]
Signed-off-by: Ard Biesheuvel 
---
 arch/x86/boot/compressed/eboot.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index dadf32312082..720b06e86698 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -123,10 +123,17 @@ __setup_efi_pci(efi_pci_io_protocol_t *pci, struct 
pci_setup_rom **__rom)
if (status != EFI_SUCCESS)
return status;
 
+   /*
+* Some firmwares contain EFI function pointers at the place where the
+* romimage and romsize fields are supposed to be. Typically the EFI
+* code is mapped at high addresses, translating to an unrealistically
+* large romsize. The UEFI spec limits the size of option ROMs to 16
+* MiB so we reject any roms over 16 MiB in size to catch this.
+*/
romimage = (void *)(unsigned long)efi_table_attr(efi_pci_io_protocol,
 romimage, pci);
romsize = efi_table_attr(efi_pci_io_protocol, romsize, pci);
-   if (!romimage || !romsize)
+   if (!romimage || !romsize || romsize > SZ_16M)
return EFI_INVALID_PARAMETER;
 
size = romsize + sizeof(*rom);
-- 
2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 3/4] efi/x86: fold __setup_efi_pci32 and __setup_efi_pci64 into one

2018-04-29 Thread Ard Biesheuvel 
As suggested by Lukas, use his efi_call_proto() and efi_table_attr()
macros to merge __setup_efi_pci32() and __setup_efi_pci64() into a
single function, removing the need to duplicate changes made in
subsequent patches across both.

Cc: Lukas Wunner 
Signed-off-by: Ard Biesheuvel 
---
 arch/x86/boot/compressed/eboot.c | 107 +---
 1 file changed, 25 insertions(+), 82 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 3994f48c4043..dadf32312082 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -109,23 +109,27 @@ void efi_char16_printk(efi_system_table_t *table, 
efi_char16_t *str)
 }
 
 static efi_status_t
-__setup_efi_pci32(efi_pci_io_protocol_32_t *pci, struct pci_setup_rom **__rom)
+__setup_efi_pci(efi_pci_io_protocol_t *pci, struct pci_setup_rom **__rom)
 {
struct pci_setup_rom *rom = NULL;
efi_status_t status;
unsigned long size;
-   uint64_t attributes;
+   uint64_t attributes, romsize;
+   void *romimage;
 
-   status = efi_early->call(pci->attributes, pci,
-EfiPciIoAttributeOperationGet, 0, 0,
-);
+   status = efi_call_proto(efi_pci_io_protocol, attributes, pci,
+   EfiPciIoAttributeOperationGet, 0, 0,
+   );
if (status != EFI_SUCCESS)
return status;
 
-   if (!pci->romimage || !pci->romsize)
+   romimage = (void *)(unsigned long)efi_table_attr(efi_pci_io_protocol,
+romimage, pci);
+   romsize = efi_table_attr(efi_pci_io_protocol, romsize, pci);
+   if (!romimage || !romsize)
return EFI_INVALID_PARAMETER;
 
-   size = pci->romsize + sizeof(*rom);
+   size = romsize + sizeof(*rom);
 
status = efi_call_early(allocate_pool, EFI_LOADER_DATA, size, );
if (status != EFI_SUCCESS) {
@@ -141,30 +145,32 @@ __setup_efi_pci32(efi_pci_io_protocol_32_t *pci, struct 
pci_setup_rom **__rom)
rom->pcilen = pci->romsize;
*__rom = rom;
 
-   status = efi_early->call(pci->pci.read, pci, EfiPciIoWidthUint16,
-PCI_VENDOR_ID, 1, &(rom->vendor));
+   status = efi_call_proto(efi_pci_io_protocol, pci.read, pci,
+   EfiPciIoWidthUint16, PCI_VENDOR_ID, 1,
+   >vendor);
 
if (status != EFI_SUCCESS) {
efi_printk(sys_table, "Failed to read rom->vendor\n");
goto free_struct;
}
 
-   status = efi_early->call(pci->pci.read, pci, EfiPciIoWidthUint16,
-PCI_DEVICE_ID, 1, &(rom->devid));
+   status = efi_call_proto(efi_pci_io_protocol, pci.read, pci,
+   EfiPciIoWidthUint16, PCI_DEVICE_ID, 1,
+   >devid);
 
if (status != EFI_SUCCESS) {
efi_printk(sys_table, "Failed to read rom->devid\n");
goto free_struct;
}
 
-   status = efi_early->call(pci->get_location, pci, &(rom->segment),
-&(rom->bus), &(rom->device), &(rom->function));
+   status = efi_call_proto(efi_pci_io_protocol, get_location, pci,
+   >segment, >bus, >device,
+   >function);
 
if (status != EFI_SUCCESS)
goto free_struct;
 
-   memcpy(rom->romdata, (void *)(unsigned long)pci->romimage,
-  pci->romsize);
+   memcpy(rom->romdata, romimage, romsize);
return status;
 
 free_struct:
@@ -176,7 +182,7 @@ static void
 setup_efi_pci32(struct boot_params *params, void **pci_handle,
unsigned long size)
 {
-   efi_pci_io_protocol_32_t *pci = NULL;
+   efi_pci_io_protocol_t *pci = NULL;
efi_guid_t pci_proto = EFI_PCI_IO_PROTOCOL_GUID;
u32 *handles = (u32 *)(unsigned long)pci_handle;
efi_status_t status;
@@ -203,7 +209,7 @@ setup_efi_pci32(struct boot_params *params, void 
**pci_handle,
if (!pci)
continue;
 
-   status = __setup_efi_pci32(pci, );
+   status = __setup_efi_pci(pci, );
if (status != EFI_SUCCESS)
continue;
 
@@ -217,74 +223,11 @@ setup_efi_pci32(struct boot_params *params, void 
**pci_handle,
}
 }
 
-static efi_status_t
-__setup_efi_pci64(efi_pci_io_protocol_64_t *pci, struct pci_setup_rom **__rom)
-{
-   struct pci_setup_rom *rom;
-   efi_status_t status;
-   unsigned long size;
-   uint64_t attributes;
-
-   status = efi_early->call(pci->attributes, pci,
-EfiPciIoAttributeOperationGet, 0,
-);
-   if (status != EFI_SUCCESS)
-   return

[PATCH v4 0/4] Ignore unrealistically large option roms in EFI stub code

2018-04-29 Thread Ard Biesheuvel 
This is a continuation of Hans's work [0] to ignore bogus romimage/romsize
values in the EFI PCI I/O protocol instances exposed by some UEFI firmwares
on x86.

I have only build tested this, both on 32 and 64 bit x86.

Changes in v4:
- Deduplicate the 32 and 64 bit code paths so that the actual change needs
  to be applied only once. This requires some preparatory work (#1, #2, #3),
  of which the first one should go to -stable.

Changes in v3:
- Limit the rom-size to 16MiB to match the EFI spec

Changes in v2:
- Add the check to both __setup_efi_pci32 and __setup_efi_pci64 instead of
  only to __setup_efi_pci64, some CHT devices which need this still use a
  32 bit UEFI

[0] https://marc.info/?l=linux-efi=152494632116321

Ard Biesheuvel (3):
  efi: fix efi_pci_io_protocol32 prototype for mixed mode
  efi: align efi_pci_io_protocol typedefs to type naming convention
  efi/x86: fold __setup_efi_pci32 and __setup_efi_pci64 into one

Hans de Goede (1):
  efi/x86: Ignore unrealistically large option roms

 arch/x86/boot/compressed/eboot.c | 112 ++--
 include/linux/efi.h  |  14 +--
 2 files changed, 39 insertions(+), 87 deletions(-)

-- 
2.17.0
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/4] efi: fix efi_pci_io_protocol32 prototype for mixed mode

2018-04-29 Thread Ard Biesheuvel 
Mixed mode allows a kernel built for x86_64 to interact with 32-bit
EFI firmware, but requires us to define all struct definitions carefully
when it comes to pointer sizes. efi_pci_io_protocol32 currently uses a
void* for the 'romimage' field, which will be interpreted as a 64-bit
field on such kernels, potentially resulting in bogus memory references
and subsequent crashes.

Cc: 
Signed-off-by: Ard Biesheuvel 
---
 arch/x86/boot/compressed/eboot.c | 6 --
 include/linux/efi.h  | 8 
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 47d3efff6805..09f36c0d9d4f 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -163,7 +163,8 @@ __setup_efi_pci32(efi_pci_io_protocol_32 *pci, struct 
pci_setup_rom **__rom)
if (status != EFI_SUCCESS)
goto free_struct;
 
-   memcpy(rom->romdata, pci->romimage, pci->romsize);
+   memcpy(rom->romdata, (void *)(unsigned long)pci->romimage,
+  pci->romsize);
return status;
 
 free_struct:
@@ -269,7 +270,8 @@ __setup_efi_pci64(efi_pci_io_protocol_64 *pci, struct 
pci_setup_rom **__rom)
if (status != EFI_SUCCESS)
goto free_struct;
 
-   memcpy(rom->romdata, pci->romimage, pci->romsize);
+   memcpy(rom->romdata, (void *)(unsigned long)pci->romimage,
+  pci->romsize);
return status;
 
 free_struct:
diff --git a/include/linux/efi.h b/include/linux/efi.h
index f1b7d68ac460..3016d8c456bc 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -395,8 +395,8 @@ typedef struct {
u32 attributes;
u32 get_bar_attributes;
u32 set_bar_attributes;
-   uint64_t romsize;
-   void *romimage;
+   u64 romsize;
+   u32 romimage;
 } efi_pci_io_protocol_32;
 
 typedef struct {
@@ -415,8 +415,8 @@ typedef struct {
u64 attributes;
u64 get_bar_attributes;
u64 set_bar_attributes;
-   uint64_t romsize;
-   void *romimage;
+   u64 romsize;
+   u64 romimage;
 } efi_pci_io_protocol_64;
 
 typedef struct {
-- 
2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] efi: Ignore unrealistically large option roms

2018-04-29 Thread Ard Biesheuvel 
On 29 April 2018 at 12:00, Ard Biesheuvel  wrote:
> On 29 April 2018 at 11:08, Hans de Goede  wrote:
>> Hi,
>>
>>
>> On 29-04-18 11:07, Ard Biesheuvel wrote:
>>>
>>> On 29 April 2018 at 10:40, Hans de Goede  wrote:

 Hi,


 On 29-04-18 09:43, Ingo Molnar wrote:
>
>
>
> * Hans de Goede  wrote:
>
>> diff --git a/arch/x86/boot/compressed/eboot.c
>> b/arch/x86/boot/compressed/eboot.c
>> index 47d3efff6805..8650ab268ee7 100644
>> --- a/arch/x86/boot/compressed/eboot.c
>> +++ b/arch/x86/boot/compressed/eboot.c
>> @@ -122,7 +122,14 @@ __setup_efi_pci32(efi_pci_io_protocol_32 *pci,
>> struct pci_setup_rom **__rom)
>>  if (status != EFI_SUCCESS)
>>  return status;
>>- if (!pci->romimage || !pci->romsize)
>> +   /*
>> +* Some firmwares contain EFI function pointers at the place
>> where the
>> +* romimage and romsize fields are supposed to be. Typically
>> the
>> EFI
>> +* code is mapped at high addresses, translating to an
>> unrealistically
>> +* large romsize. The UEFI spec limits the size of option ROMs
>> to
>> 16
>> +* MiB so we reject any roms over 16 MiB in size to catch this.
>> +*/
>> +   if (!pci->romimage || !pci->romsize || pci->romsize >
>> 0x100)
>>  return EFI_INVALID_PARAMETER;
>>  size = pci->romsize + sizeof(*rom);
>> @@ -230,7 +237,14 @@ __setup_efi_pci64(efi_pci_io_protocol_64 *pci,
>> struct pci_setup_rom **__rom)
>>  if (status != EFI_SUCCESS)
>>  return status;
>>- if (!pci->romimage || !pci->romsize)
>> +   /*
>> +* Some firmwares contain EFI function pointers at the place
>> where the
>> +* romimage and romsize fields are supposed to be. Typically
>> the
>> EFI
>> +* code is mapped at high addresses, translating to an
>> unrealistically
>> +* large romsize. The UEFI spec limits the size of option ROMs
>> to
>> 16
>> +* MiB so we reject any roms over 16 MiB in size to catch this.
>> +*/
>> +   if (!pci->romimage || !pci->romsize || pci->romsize >
>> 0x100)
>>  return EFI_INVALID_PARAMETER;
>
>
>
> Any reason why this couldn't be factored out into a efi_check_rom(pci)
> kind of
> helper function



 The pci pointer is of 2 different types:

 __setup_efi_pci32(efi_pci_io_protocol_32 *pci, ...
 __setup_efi_pci64(efi_pci_io_protocol_64 *pci, ...

 I guess I could give the helper a romimage and romsize argument to get
 around that.
 Ard, do you want me to do a v4 with such a helper ?

>>>
>>> I think Lukas has a point, although I shouldn't expect you to implement
>>> that.
>>>
>>> I will look into this myself, and rebase your patch on top of that.
>>
>>
>> Ok, thanks.
>>
>
> Actually, looking into this, I think I may have spotted a bug in this code.
> Does the issue you are solving here occur when running a 64-bit kernel
> on 32-bit EFI?

Never mind. I was looking at romimage being defined as a void* in
efi_pci_io_protocol32, which is a bug, but the romsize field is
unambiguously defined as a uint64_t, and this is the field holding the
bogus value.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] efi: Ignore unrealistically large option roms

2018-04-29 Thread Ard Biesheuvel 
On 29 April 2018 at 11:08, Hans de Goede  wrote:
> Hi,
>
>
> On 29-04-18 11:07, Ard Biesheuvel wrote:
>>
>> On 29 April 2018 at 10:40, Hans de Goede  wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 29-04-18 09:43, Ingo Molnar wrote:



 * Hans de Goede  wrote:

> diff --git a/arch/x86/boot/compressed/eboot.c
> b/arch/x86/boot/compressed/eboot.c
> index 47d3efff6805..8650ab268ee7 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -122,7 +122,14 @@ __setup_efi_pci32(efi_pci_io_protocol_32 *pci,
> struct pci_setup_rom **__rom)
>  if (status != EFI_SUCCESS)
>  return status;
>- if (!pci->romimage || !pci->romsize)
> +   /*
> +* Some firmwares contain EFI function pointers at the place
> where the
> +* romimage and romsize fields are supposed to be. Typically
> the
> EFI
> +* code is mapped at high addresses, translating to an
> unrealistically
> +* large romsize. The UEFI spec limits the size of option ROMs
> to
> 16
> +* MiB so we reject any roms over 16 MiB in size to catch this.
> +*/
> +   if (!pci->romimage || !pci->romsize || pci->romsize >
> 0x100)
>  return EFI_INVALID_PARAMETER;
>  size = pci->romsize + sizeof(*rom);
> @@ -230,7 +237,14 @@ __setup_efi_pci64(efi_pci_io_protocol_64 *pci,
> struct pci_setup_rom **__rom)
>  if (status != EFI_SUCCESS)
>  return status;
>- if (!pci->romimage || !pci->romsize)
> +   /*
> +* Some firmwares contain EFI function pointers at the place
> where the
> +* romimage and romsize fields are supposed to be. Typically
> the
> EFI
> +* code is mapped at high addresses, translating to an
> unrealistically
> +* large romsize. The UEFI spec limits the size of option ROMs
> to
> 16
> +* MiB so we reject any roms over 16 MiB in size to catch this.
> +*/
> +   if (!pci->romimage || !pci->romsize || pci->romsize >
> 0x100)
>  return EFI_INVALID_PARAMETER;



 Any reason why this couldn't be factored out into a efi_check_rom(pci)
 kind of
 helper function
>>>
>>>
>>>
>>> The pci pointer is of 2 different types:
>>>
>>> __setup_efi_pci32(efi_pci_io_protocol_32 *pci, ...
>>> __setup_efi_pci64(efi_pci_io_protocol_64 *pci, ...
>>>
>>> I guess I could give the helper a romimage and romsize argument to get
>>> around that.
>>> Ard, do you want me to do a v4 with such a helper ?
>>>
>>
>> I think Lukas has a point, although I shouldn't expect you to implement
>> that.
>>
>> I will look into this myself, and rebase your patch on top of that.
>
>
> Ok, thanks.
>

Actually, looking into this, I think I may have spotted a bug in this code.
Does the issue you are solving here occur when running a 64-bit kernel
on 32-bit EFI?
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 1/5] efi: Export boot-services code and data as debugfs-blobs

2018-04-29 Thread Hans de Goede 
Sometimes it is useful to be able to dump the efi boot-services code and
data. This commit adds these as debugfs-blobs to /sys/kernel/debug/efi,
but only if efi=debug is passed on the kernel-commandline as this requires
not freeing those memory-regions, which costs 20+ MB of RAM.

Reviewed-by: Greg Kroah-Hartman 
Acked-by: Ard Biesheuvel 
Signed-off-by: Hans de Goede 
---
Changes in v5:
-Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS

Changes in v4:
-Add new EFI_BOOT_SERVICES flag and use it to determine if the boot-services
 memory segments are available (and thus if it makes sense to register the
 debugfs bits for them)

Changes in v2:
-Do not call pr_err on debugfs call failures
---
 arch/x86/platform/efi/efi.c|  1 +
 arch/x86/platform/efi/quirks.c |  4 +++
 drivers/firmware/efi/efi.c | 53 ++
 include/linux/efi.h|  1 +
 4 files changed, 59 insertions(+)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 9061babfbc83..82f0836d 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -208,6 +208,7 @@ int __init efi_memblock_x86_reserve_range(void)
 efi.memmap.desc_version);
 
memblock_reserve(pmap, efi.memmap.nr_map * efi.memmap.desc_size);
+   set_bit(EFI_PRESERVE_BS_REGIONS, );
 
return 0;
 }
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 36c1f8b9f7e0..16bdb9e3b343 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -376,6 +376,10 @@ void __init efi_free_boot_services(void)
int num_entries = 0;
void *new, *new_md;
 
+   /* Keep all regions for /sys/kernel/debug/efi */
+   if (efi_enabled(EFI_DBG))
+   return;
+
for_each_efi_memory_desc(md) {
unsigned long long start = md->phys_addr;
unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 232f4915223b..1590e4d6a857 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -325,6 +326,55 @@ static __init int efivar_ssdt_load(void)
 static inline int efivar_ssdt_load(void) { return 0; }
 #endif
 
+#ifdef CONFIG_DEBUG_FS
+
+#define EFI_DEBUGFS_MAX_BLOBS 32
+
+static struct debugfs_blob_wrapper debugfs_blob[EFI_DEBUGFS_MAX_BLOBS];
+
+static void __init efi_debugfs_init(void)
+{
+   struct dentry *efi_debugfs;
+   efi_memory_desc_t *md;
+   char name[32];
+   int type_count[EFI_BOOT_SERVICES_DATA + 1] = {};
+   int i = 0;
+
+   efi_debugfs = debugfs_create_dir("efi", NULL);
+   if (IS_ERR_OR_NULL(efi_debugfs))
+   return;
+
+   for_each_efi_memory_desc(md) {
+   switch (md->type) {
+   case EFI_BOOT_SERVICES_CODE:
+   snprintf(name, sizeof(name), "boot_services_code%d",
+type_count[md->type]++);
+   break;
+   case EFI_BOOT_SERVICES_DATA:
+   snprintf(name, sizeof(name), "boot_services_data%d",
+type_count[md->type]++);
+   break;
+   default:
+   continue;
+   }
+
+   debugfs_blob[i].size = md->num_pages << EFI_PAGE_SHIFT;
+   debugfs_blob[i].data = memremap(md->phys_addr,
+   debugfs_blob[i].size,
+   MEMREMAP_WB);
+   if (!debugfs_blob[i].data)
+   continue;
+
+   debugfs_create_blob(name, 0400, efi_debugfs, _blob[i]);
+   i++;
+   if (i == EFI_DEBUGFS_MAX_BLOBS)
+   break;
+   }
+}
+#else
+static inline void efi_debugfs_init(void) {}
+#endif
+
 /*
  * We register the efi subsystem with the firmware subsystem and the
  * efivars subsystem with the efi subsystem, if the system was booted with
@@ -369,6 +419,9 @@ static int __init efisubsys_init(void)
goto err_remove_group;
}
 
+   if (efi_enabled(EFI_DBG) && efi_enabled(EFI_PRESERVE_BS_REGIONS))
+   efi_debugfs_init();
+
return 0;
 
 err_remove_group:
diff --git a/include/linux/efi.h b/include/linux/efi.h
index f1b7d68ac460..791088360c1e 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1144,6 +1144,7 @@ extern int __init efi_setup_pcdp_console(char *);
 #define EFI_DBG8   /* Print additional debug info 
at runtime */
 #define EFI_NX_PE_DATA 9   /* Can runtime data regions be mapped 
non-executable? */
 #define EFI_MEM_ATTR   10  /* Did firmware publish an 
EFI_MEMORY_ATTRIBUTES

[PATCH v5 2/5] efi: Add embedded peripheral firmware support

2018-04-29 Thread Hans de Goede 
Just like with PCI options ROMs, which we save in the setup_efi_pci*
functions from arch/x86/boot/compressed/eboot.c, the EFI code / ROM itself
sometimes may contain data which is useful/necessary for peripheral drivers
to have access to.

Specifically the EFI code may contain an embedded copy of firmware which
needs to be (re)loaded into the peripheral. Normally such firmware would be
part of linux-firmware, but in some cases this is not feasible, for 2
reasons:

1) The firmware is customized for a specific use-case of the chipset / use
with a specific hardware model, so we cannot have a single firmware file
for the chipset. E.g. touchscreen controller firmwares are compiled
specifically for the hardware model they are used with, as they are
calibrated for a specific model digitizer.

2) Despite repeated attempts we have failed to get permission to
redistribute the firmware. This is especially a problem with customized
firmwares, these get created by the chip vendor for a specific ODM and the
copyright may partially belong with the ODM, so the chip vendor cannot
give a blanket permission to distribute these.

This commit adds support for finding peripheral firmware embedded in the
EFI code and making this available to peripheral drivers through the
standard firmware loading mechanism.

Note we check the EFI_BOOT_SERVICES_CODE for embedded firmware near the end
of start_kernel(), just before calling rest_init(), this is on purpose
because the typical EFI_BOOT_SERVICES_CODE memory-segment is too large for
early_memremap(), so the check must be done after mm_init(). This relies
on EFI_BOOT_SERVICES_CODE not being free-ed until efi_free_boot_services()
is called, which means that this will only work on x86 for now.

Reported-by: Dave Olsthoorn 
Suggested-by: Peter Jones 
Acked-by: Ard Biesheuvel 
Signed-off-by: Hans de Goede 
---
Changes in v5:
-Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS

Changes in v4:
-Drop note in docs about EFI_FIRMWARE_VOLUME_PROTOCOL, it is not part of
 UEFI proper, so the EFI maintainers don't want us referring people to it
-Use new EFI_BOOT_SERVICES flag
-Put the new fw_get_efi_embedded_fw() function in its own fallback_efi.c
 file which only gets built when EFI_EMBEDDED_FIRMWARE is selected
-Define an empty stub for fw_get_efi_embedded_fw() in fallback.h hwen
 EFI_EMBEDDED_FIRMWARE is not selected, to avoid the need for #ifdefs
 in firmware_loader/main.c
-Properly call security_kernel_post_read_file() on the firmware returned
 by efi_get_embedded_fw() to make sure that we are allowed to use it

Changes in v3:
-Fix the docs using "efi-embedded-fw" as property name instead of
 "efi-embedded-firmware"

Changes in v2:
-Rebased on driver-core/driver-core-next
-Add documentation describing the EFI embedded firmware mechanism to:
 Documentation/driver-api/firmware/request_firmware.rst
-Add a new EFI_EMBEDDED_FIRMWARE Kconfig bool and only build the embedded
 fw support if this is set. This is an invisible option which should be
 selected by drivers which need this
-Remove the efi_embedded_fw_desc and dmi_system_id-s for known devices
 from the efi-embedded-fw code, instead drivers using this are expected to
 export a dmi_system_id array, with each entries' driver_data pointing to a
 efi_embedded_fw_desc struct and register this with the efi-embedded-fw code
-Use kmemdup to make a copy instead of efi_mem_reserve()-ing the firmware,
 this avoids us messing with the EFI memmap and avoids the need to make
 changes to efi_mem_desc_lookup()
-Make the firmware-loader code only fallback to efi_get_embedded_fw() if the
 passed in device has the "efi-embedded-firmware" device-property bool set
-Skip usermodehelper fallback when "efi-embedded-firmware" device-property
 is set
---
 .../driver-api/firmware/request_firmware.rst  |  66 
 drivers/base/firmware_loader/Makefile |   1 +
 drivers/base/firmware_loader/fallback.h   |  12 ++
 drivers/base/firmware_loader/fallback_efi.c   |  51 ++
 drivers/base/firmware_loader/main.c   |   2 +
 drivers/firmware/efi/Kconfig  |   3 +
 drivers/firmware/efi/Makefile |   1 +
 drivers/firmware/efi/embedded-firmware.c  | 149 ++
 include/linux/efi.h   |   6 +
 include/linux/efi_embedded_fw.h   |  25 +++
 init/main.c   |   3 +
 11 files changed, 319 insertions(+)
 create mode 100644 drivers/base/firmware_loader/fallback_efi.c
 create mode 100644 drivers/firmware/efi/embedded-firmware.c
 create mode 100644 include/linux/efi_embedded_fw.h

diff --git a/Documentation/driver-api/firmware/request_firmware.rst 
b/Documentation/driver-api/firmware/request_firmware.rst
index c8bddbdcfd10..560dfed76e38 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@

[PATCH v5 5/5] platform/x86: touchscreen_dmi: Add info for the Chuwi Vi8 Plus tablet

2018-04-29 Thread Hans de Goede 
Add touchscreen info for the Chuwi Vi8 Plus tablet. This tablet uses a
Chipone ICN8505 touchscreen controller, with the firmware used by the
touchscreen embedded in the EFI firmware.

Acked-by: Andy Shevchenko 
Acked-by: Ard Biesheuvel 
Signed-off-by: Hans de Goede 
---
 drivers/platform/x86/touchscreen_dmi.c | 25 +
 1 file changed, 25 insertions(+)

diff --git a/drivers/platform/x86/touchscreen_dmi.c 
b/drivers/platform/x86/touchscreen_dmi.c
index 6488cd50ba79..5fdb6fe878f4 100644
--- a/drivers/platform/x86/touchscreen_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -301,6 +301,22 @@ static const struct ts_dmi_data teclast_x3_plus_data = {
.properties = teclast_x3_plus_props,
 };
 
+static const struct property_entry efi_embedded_fw_props[] = {
+   PROPERTY_ENTRY_BOOL("efi-embedded-firmware"),
+   { }
+};
+
+static const struct ts_dmi_data chuwi_vi8_plus_data = {
+   .embedded_fw = {
+   .name   = "chipone/icn8505-HAMP0002.fw",
+   .prefix = { 0xb0, 0x07, 0x00, 0x00, 0xe4, 0x07, 0x00, 0x00 },
+   .length = 35012,
+   .crc= 0x74dfd3fc,
+   },
+   .acpi_name  = "CHPN0001:00",
+   .properties = efi_embedded_fw_props,
+};
+
 const struct dmi_system_id touchscreen_dmi_table[] = {
{
/* CUBE iwork8 Air */
@@ -487,6 +503,15 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
DMI_MATCH(DMI_PRODUCT_NAME, "Y8W81"),
},
},
+   {
+   /* Chuwi Vi8 Plus (CWI506) */
+   .driver_data = (void *)_vi8_plus_data,
+   .matches = {
+   DMI_MATCH(DMI_SYS_VENDOR, "Hampoo"),
+   DMI_MATCH(DMI_PRODUCT_NAME, "D2D3_Vi8A1"),
+   DMI_MATCH(DMI_BOARD_NAME, "Cherry Trail CR"),
+   },
+   },
{ },
 };
 
-- 
2.17.0

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 4/5] platform/x86: touchscreen_dmi: Add EFI embedded firmware info support

2018-04-29 Thread Hans de Goede 
Sofar we have been unable to get permission from the vendors to put the
firmware for touchscreens listed in touchscreen_dmi in linux-firmware.

Some of the tablets with such a touchscreen have a touchscreen driver, and
thus a copy of the firmware, as part of their EFI code.

This commit adds the necessary info for the new EFI embedded-firmware code
to extract these firmwares, making the touchscreen work OOTB without the
user needing to manually add the firmware.

Acked-by: Andy Shevchenko 
Acked-by: Ard Biesheuvel 
Signed-off-by: Hans de Goede 
---
 drivers/firmware/efi/embedded-firmware.c |  3 +++
 drivers/platform/x86/Kconfig |  1 +
 drivers/platform/x86/touchscreen_dmi.c   | 26 +++-
 include/linux/efi_embedded_fw.h  |  2 ++
 4 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/embedded-firmware.c 
b/drivers/firmware/efi/embedded-firmware.c
index 22a0f598b53d..36a93a6938f0 100644
--- a/drivers/firmware/efi/embedded-firmware.c
+++ b/drivers/firmware/efi/embedded-firmware.c
@@ -23,6 +23,9 @@ struct embedded_fw {
 static LIST_HEAD(found_fw_list);
 
 static const struct dmi_system_id * const embedded_fw_table[] = {
+#ifdef CONFIG_TOUCHSCREEN_DMI
+   touchscreen_dmi_table,
+#endif
NULL
 };
 
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 7ff206e5edfe..cc4d95459190 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1199,6 +1199,7 @@ config INTEL_TURBO_MAX_3
 config TOUCHSCREEN_DMI
bool "DMI based touchscreen configuration info"
depends on ACPI && DMI && I2C=y && TOUCHSCREEN_SILEAD
+   select EFI_EMBEDDED_FIRMWARE if EFI
---help---
  Certain ACPI based tablets with e.g. Silead or Chipone touchscreens
  do not have enough data in ACPI tables for the touchscreen driver to
diff --git a/drivers/platform/x86/touchscreen_dmi.c 
b/drivers/platform/x86/touchscreen_dmi.c
index 87fc839b28f7..6488cd50ba79 100644
--- a/drivers/platform/x86/touchscreen_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -15,12 +15,15 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
 
 struct ts_dmi_data {
+   /* The EFI embedded-fw code expects this to be the first member! */
+   struct efi_embedded_fw_desc embedded_fw;
const char *acpi_name;
const struct property_entry *properties;
 };
@@ -31,10 +34,17 @@ static const struct property_entry cube_iwork8_air_props[] 
= {
PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
PROPERTY_ENTRY_STRING("firmware-name", "gsl3670-cube-iwork8-air.fw"),
PROPERTY_ENTRY_U32("silead,max-fingers", 10),
+   PROPERTY_ENTRY_BOOL("efi-embedded-firmware"),
{ }
 };
 
 static const struct ts_dmi_data cube_iwork8_air_data = {
+   .embedded_fw = {
+   .name   = "silead/gsl3670-cube-iwork8-air.fw",
+   .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
+   .length = 38808,
+   .crc= 0xfecde51f,
+   },
.acpi_name  = "MSSL1680:00",
.properties = cube_iwork8_air_props,
 };
@@ -119,10 +129,17 @@ static const struct property_entry pipo_w2s_props[] = {
PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
PROPERTY_ENTRY_STRING("firmware-name",
  "gsl1680-pipo-w2s.fw"),
+   PROPERTY_ENTRY_BOOL("efi-embedded-firmware"),
{ }
 };
 
 static const struct ts_dmi_data pipo_w2s_data = {
+   .embedded_fw = {
+   .name   = "silead/gsl1680-pipo-w2s.fw",
+   .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
+   .length = 39072,
+   .crc= 0x28d5dc6c,
+   },
.acpi_name  = "MSSL1680:00",
.properties = pipo_w2s_props,
 };
@@ -162,10 +179,17 @@ static const struct property_entry chuwi_hi8_pro_props[] 
= {
PROPERTY_ENTRY_BOOL("touchscreen-swapped-x-y"),
PROPERTY_ENTRY_STRING("firmware-name", "gsl3680-chuwi-hi8-pro.fw"),
PROPERTY_ENTRY_BOOL("silead,home-button"),
+   PROPERTY_ENTRY_BOOL("efi-embedded-firmware"),
{ }
 };
 
 static const struct ts_dmi_data chuwi_hi8_pro_data = {
+   .embedded_fw = {
+   .name   = "silead/gsl3680-chuwi-hi8-pro.fw",
+   .prefix = { 0xf0, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00 },
+   .length = 39864,
+   .crc= 0xfe2bedba,
+   },
.acpi_name  = "MSSL1680:00",
.properties = chuwi_hi8_pro_props,
 };
@@ -277,7 +301,7 @@ static const struct ts_dmi_data teclast_x3_plus_data = {
.properties = teclast_x3_plus_props,
 };
 
-static const struct dmi_system_id touchscreen_dmi_table[] = {
+const struct dmi_system_id touchscreen_dmi_table[] = {
{
/* CUBE iwork8 Air */

[PATCH v5 0/5] efi/firmware/platform-x86: Add EFI embedded fw support

2018-04-29 Thread Hans de Goede 
Hi All,

Here is v5 of my patch-set to add support for EFI embedded fw to the kernel.

Changes since v4:
-Rename the EFI_BOOT_SERVICES flag to EFI_PRESERVE_BS_REGIONS

So I think this patch-set is getting close to ready for merging, which
brings us to the question of how to merge this, I think that patches 1
and 2 should probably both be merged through the same tree. Then an
unmutable branch should be created on that tree, merged into the
platform/x86 tree and then the last 3 patches can be merged through
that tree.

Ard has already indicated he is fine with the EFI bits going upstream
through another tree, so perhaps patches 1-2 can be merged through the
firmware-loader-tree and then do an unmutable branch on the
firmware-loader-tree for the platform/x86 tree to merge?

I don't think taking all 5 through 1 tree is a good idea because of
the file rename under platform/x86.


For the record here are the cover letters of the previous versions:

Changes since v3:
-Drop note in docs about EFI_FIRMWARE_VOLUME_PROTOCOL, it is not part of
 UEFI proper, so the EFI maintainers don't want us referring people to it
-Use new EFI_BOOT_SERVICES flag
-Put the new fw_get_efi_embedded_fw() function in its own fallback_efi.c
 file which only gets built when EFI_EMBEDDED_FIRMWARE is selected
-Define an empty stub for fw_get_efi_embedded_fw() in fallback.h hwen
 EFI_EMBEDDED_FIRMWARE is not selected, to avoid the need for #ifdefs
 in firmware_loader/main.c
-Properly call security_kernel_post_read_file() on the firmware returned
 by efi_get_embedded_fw() to make sure that we are allowed to use it

The 3 most prominent changes in v2 are:

1) Add documentation describing the EFI embedded firmware mechanism to:
   Documentation/driver-api/firmware/request_firmware.rst

2) Instead of having a single dmi_system_id array with its driver_data
   members pointing to efi_embedded_fw_desc structs, have the drivers which
   need EFI embedded-fw support export a dmi_system_id array and register
   that with the EFI embedded-fw code

   This series also includes the first driver to use this, in the form of
   the touchscreen_dmi code (formerly silead_dmi) from drivers/platfrom/x86

3) As discussed during the review of v1 we want to make the firmware_loader
   code fallback to EFI embedded-fw optional.  Rather the adding yet another
   firmware_request_foo variant for this, with the risk of later also needing
   firmware_request_foo_nowait, etc. variants I've decided to make the code
   check if the device has a "efi-embedded-firmware" device-property bool set.

   This also seemed better because the same driver may want to use the
   fallback on some systems, but not on others since e.g. not all (x86)
   systems with a silead touchscreen have their touchscreen firmware embedded
   in their EFI.

   Note that (as discussed) when the EFI fallback path is requested, the
   usermodehelper fallback path is skipped.

Here is the full changelog of patch 2/5 which is where most of the changes are:

Changes in v2:
-Rebased on driver-core/driver-core-next
-Add documentation describing the EFI embedded firmware mechanism to:
 Documentation/driver-api/firmware/request_firmware.rst
-Add a new EFI_EMBEDDED_FIRMWARE Kconfig bool and only build the embedded
 fw support if this is set. This is an invisible option which should be
 selected by drivers which need this
-Remove the efi_embedded_fw_desc and dmi_system_id-s for known devices
 from the efi-embedded-fw code, instead drivers using this are expected to
 export a dmi_system_id array, with each entries' driver_data pointing to a
 efi_embedded_fw_desc struct and register this with the efi-embedded-fw code
-Use kmemdup to make a copy instead of efi_mem_reserve()-ing the firmware,
 this avoids us messing with the EFI memmap and avoids the need to make
 changes to efi_mem_desc_lookup()
-Make the firmware-loader code only fallback to efi_get_embedded_fw() if the
 passed in device has the "efi-embedded-firmware" device-property bool set
-Skip usermodehelper fallback when "efi-embedded-firmware" device-property
 is set

Patches 3-5 are new and implement using the EFI embedded-fw mechanism for
Silead gsl and Chipone icn8505 touchscreens on x86 devices.

Regards,

Hans

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] efi: Ignore unrealistically large option roms

2018-04-29 Thread Hans de Goede 

Hi,

On 29-04-18 11:07, Ard Biesheuvel wrote:

On 29 April 2018 at 10:40, Hans de Goede  wrote:

Hi,


On 29-04-18 09:43, Ingo Molnar wrote:



* Hans de Goede  wrote:


diff --git a/arch/x86/boot/compressed/eboot.c
b/arch/x86/boot/compressed/eboot.c
index 47d3efff6805..8650ab268ee7 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -122,7 +122,14 @@ __setup_efi_pci32(efi_pci_io_protocol_32 *pci,
struct pci_setup_rom **__rom)
 if (status != EFI_SUCCESS)
 return status;
   - if (!pci->romimage || !pci->romsize)
+   /*
+* Some firmwares contain EFI function pointers at the place
where the
+* romimage and romsize fields are supposed to be. Typically the
EFI
+* code is mapped at high addresses, translating to an
unrealistically
+* large romsize. The UEFI spec limits the size of option ROMs to
16
+* MiB so we reject any roms over 16 MiB in size to catch this.
+*/
+   if (!pci->romimage || !pci->romsize || pci->romsize > 0x100)
 return EFI_INVALID_PARAMETER;
 size = pci->romsize + sizeof(*rom);
@@ -230,7 +237,14 @@ __setup_efi_pci64(efi_pci_io_protocol_64 *pci,
struct pci_setup_rom **__rom)
 if (status != EFI_SUCCESS)
 return status;
   - if (!pci->romimage || !pci->romsize)
+   /*
+* Some firmwares contain EFI function pointers at the place
where the
+* romimage and romsize fields are supposed to be. Typically the
EFI
+* code is mapped at high addresses, translating to an
unrealistically
+* large romsize. The UEFI spec limits the size of option ROMs to
16
+* MiB so we reject any roms over 16 MiB in size to catch this.
+*/
+   if (!pci->romimage || !pci->romsize || pci->romsize > 0x100)
 return EFI_INVALID_PARAMETER;



Any reason why this couldn't be factored out into a efi_check_rom(pci)
kind of
helper function



The pci pointer is of 2 different types:

__setup_efi_pci32(efi_pci_io_protocol_32 *pci, ...
__setup_efi_pci64(efi_pci_io_protocol_64 *pci, ...

I guess I could give the helper a romimage and romsize argument to get
around that.
Ard, do you want me to do a v4 with such a helper ?



I think Lukas has a point, although I shouldn't expect you to implement that.

I will look into this myself, and rebase your patch on top of that.


Ok, thanks.

Regards,

Hans

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] efi: Ignore unrealistically large option roms

2018-04-29 Thread Ard Biesheuvel 
On 29 April 2018 at 10:40, Hans de Goede  wrote:
> Hi,
>
>
> On 29-04-18 09:43, Ingo Molnar wrote:
>>
>>
>> * Hans de Goede  wrote:
>>
>>> diff --git a/arch/x86/boot/compressed/eboot.c
>>> b/arch/x86/boot/compressed/eboot.c
>>> index 47d3efff6805..8650ab268ee7 100644
>>> --- a/arch/x86/boot/compressed/eboot.c
>>> +++ b/arch/x86/boot/compressed/eboot.c
>>> @@ -122,7 +122,14 @@ __setup_efi_pci32(efi_pci_io_protocol_32 *pci,
>>> struct pci_setup_rom **__rom)
>>> if (status != EFI_SUCCESS)
>>> return status;
>>>   - if (!pci->romimage || !pci->romsize)
>>> +   /*
>>> +* Some firmwares contain EFI function pointers at the place
>>> where the
>>> +* romimage and romsize fields are supposed to be. Typically the
>>> EFI
>>> +* code is mapped at high addresses, translating to an
>>> unrealistically
>>> +* large romsize. The UEFI spec limits the size of option ROMs to
>>> 16
>>> +* MiB so we reject any roms over 16 MiB in size to catch this.
>>> +*/
>>> +   if (!pci->romimage || !pci->romsize || pci->romsize > 0x100)
>>> return EFI_INVALID_PARAMETER;
>>> size = pci->romsize + sizeof(*rom);
>>> @@ -230,7 +237,14 @@ __setup_efi_pci64(efi_pci_io_protocol_64 *pci,
>>> struct pci_setup_rom **__rom)
>>> if (status != EFI_SUCCESS)
>>> return status;
>>>   - if (!pci->romimage || !pci->romsize)
>>> +   /*
>>> +* Some firmwares contain EFI function pointers at the place
>>> where the
>>> +* romimage and romsize fields are supposed to be. Typically the
>>> EFI
>>> +* code is mapped at high addresses, translating to an
>>> unrealistically
>>> +* large romsize. The UEFI spec limits the size of option ROMs to
>>> 16
>>> +* MiB so we reject any roms over 16 MiB in size to catch this.
>>> +*/
>>> +   if (!pci->romimage || !pci->romsize || pci->romsize > 0x100)
>>> return EFI_INVALID_PARAMETER;
>>
>>
>> Any reason why this couldn't be factored out into a efi_check_rom(pci)
>> kind of
>> helper function
>
>
> The pci pointer is of 2 different types:
>
> __setup_efi_pci32(efi_pci_io_protocol_32 *pci, ...
> __setup_efi_pci64(efi_pci_io_protocol_64 *pci, ...
>
> I guess I could give the helper a romimage and romsize argument to get
> around that.
> Ard, do you want me to do a v4 with such a helper ?
>

I think Lukas has a point, although I shouldn't expect you to implement that.

I will look into this myself, and rebase your patch on top of that.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] efi: Ignore unrealistically large option roms

2018-04-29 Thread Hans de Goede 

Hi,

On 29-04-18 09:43, Ingo Molnar wrote:


* Hans de Goede  wrote:


diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 47d3efff6805..8650ab268ee7 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -122,7 +122,14 @@ __setup_efi_pci32(efi_pci_io_protocol_32 *pci, struct 
pci_setup_rom **__rom)
if (status != EFI_SUCCESS)
return status;
  
-	if (!pci->romimage || !pci->romsize)

+   /*
+* Some firmwares contain EFI function pointers at the place where the
+* romimage and romsize fields are supposed to be. Typically the EFI
+* code is mapped at high addresses, translating to an unrealistically
+* large romsize. The UEFI spec limits the size of option ROMs to 16
+* MiB so we reject any roms over 16 MiB in size to catch this.
+*/
+   if (!pci->romimage || !pci->romsize || pci->romsize > 0x100)
return EFI_INVALID_PARAMETER;
  
  	size = pci->romsize + sizeof(*rom);

@@ -230,7 +237,14 @@ __setup_efi_pci64(efi_pci_io_protocol_64 *pci, struct 
pci_setup_rom **__rom)
if (status != EFI_SUCCESS)
return status;
  
-	if (!pci->romimage || !pci->romsize)

+   /*
+* Some firmwares contain EFI function pointers at the place where the
+* romimage and romsize fields are supposed to be. Typically the EFI
+* code is mapped at high addresses, translating to an unrealistically
+* large romsize. The UEFI spec limits the size of option ROMs to 16
+* MiB so we reject any roms over 16 MiB in size to catch this.
+*/
+   if (!pci->romimage || !pci->romsize || pci->romsize > 0x100)
return EFI_INVALID_PARAMETER;


Any reason why this couldn't be factored out into a efi_check_rom(pci) kind of
helper function


The pci pointer is of 2 different types:

__setup_efi_pci32(efi_pci_io_protocol_32 *pci, ...
__setup_efi_pci64(efi_pci_io_protocol_64 *pci, ...

I guess I could give the helper a romimage and romsize argument to get around 
that.
Ard, do you want me to do a v4 with such a helper ?

Regards,

Hans


--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] efi: Ignore unrealistically large option roms

2018-04-29 Thread Lukas Wunner 
On Sun, Apr 29, 2018 at 09:43:18AM +0200, Ingo Molnar wrote:
> * Hans de Goede  wrote:
> > diff --git a/arch/x86/boot/compressed/eboot.c 
> > b/arch/x86/boot/compressed/eboot.c
> > index 47d3efff6805..8650ab268ee7 100644
> > --- a/arch/x86/boot/compressed/eboot.c
> > +++ b/arch/x86/boot/compressed/eboot.c
> > @@ -122,7 +122,14 @@ __setup_efi_pci32(efi_pci_io_protocol_32 *pci, struct 
> > pci_setup_rom **__rom)
> > if (status != EFI_SUCCESS)
> > return status;
> >  
> > -   if (!pci->romimage || !pci->romsize)
> > +   /*
> > +* Some firmwares contain EFI function pointers at the place where the
> > +* romimage and romsize fields are supposed to be. Typically the EFI
> > +* code is mapped at high addresses, translating to an unrealistically
> > +* large romsize. The UEFI spec limits the size of option ROMs to 16
> > +* MiB so we reject any roms over 16 MiB in size to catch this.
> > +*/
> > +   if (!pci->romimage || !pci->romsize || pci->romsize > 0x100)
> > return EFI_INVALID_PARAMETER;
> >  
> > size = pci->romsize + sizeof(*rom);
> > @@ -230,7 +237,14 @@ __setup_efi_pci64(efi_pci_io_protocol_64 *pci, struct 
> > pci_setup_rom **__rom)
> > if (status != EFI_SUCCESS)
> > return status;
> >  
> > -   if (!pci->romimage || !pci->romsize)
> > +   /*
> > +* Some firmwares contain EFI function pointers at the place where the
> > +* romimage and romsize fields are supposed to be. Typically the EFI
> > +* code is mapped at high addresses, translating to an unrealistically
> > +* large romsize. The UEFI spec limits the size of option ROMs to 16
> > +* MiB so we reject any roms over 16 MiB in size to catch this.
> > +*/
> > +   if (!pci->romimage || !pci->romsize || pci->romsize > 0x100)
> > return EFI_INVALID_PARAMETER;
> 
> Any reason why this couldn't be factored out into a efi_check_rom(pci)
> kind of helper function, which would unify the logic and would also
> avoid the duplicate comment blocks?

The real fix would be to deduplicate __setup_efi_pci32 and __setup_efi_pci64
à la commits 2bd79f30eea1 and db4545d9a788.

(That said the comment seems overly wordy.  A short pointer to the UEFI spec
with chapter number should be sufficient, anything else should be in the
changelog.  Just my 2 cents anyway.)

Thanks,

Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] efi: Ignore unrealistically large option roms

2018-04-29 Thread Ingo Molnar 

* Hans de Goede  wrote:

> diff --git a/arch/x86/boot/compressed/eboot.c 
> b/arch/x86/boot/compressed/eboot.c
> index 47d3efff6805..8650ab268ee7 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -122,7 +122,14 @@ __setup_efi_pci32(efi_pci_io_protocol_32 *pci, struct 
> pci_setup_rom **__rom)
>   if (status != EFI_SUCCESS)
>   return status;
>  
> - if (!pci->romimage || !pci->romsize)
> + /*
> +  * Some firmwares contain EFI function pointers at the place where the
> +  * romimage and romsize fields are supposed to be. Typically the EFI
> +  * code is mapped at high addresses, translating to an unrealistically
> +  * large romsize. The UEFI spec limits the size of option ROMs to 16
> +  * MiB so we reject any roms over 16 MiB in size to catch this.
> +  */
> + if (!pci->romimage || !pci->romsize || pci->romsize > 0x100)
>   return EFI_INVALID_PARAMETER;
>  
>   size = pci->romsize + sizeof(*rom);
> @@ -230,7 +237,14 @@ __setup_efi_pci64(efi_pci_io_protocol_64 *pci, struct 
> pci_setup_rom **__rom)
>   if (status != EFI_SUCCESS)
>   return status;
>  
> - if (!pci->romimage || !pci->romsize)
> + /*
> +  * Some firmwares contain EFI function pointers at the place where the
> +  * romimage and romsize fields are supposed to be. Typically the EFI
> +  * code is mapped at high addresses, translating to an unrealistically
> +  * large romsize. The UEFI spec limits the size of option ROMs to 16
> +  * MiB so we reject any roms over 16 MiB in size to catch this.
> +  */
> + if (!pci->romimage || !pci->romsize || pci->romsize > 0x100)
>   return EFI_INVALID_PARAMETER;

Any reason why this couldn't be factored out into a efi_check_rom(pci) kind of 
helper function, which would unify the logic and would also avoid the duplicate 
comment blocks?

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html