[PATCH 5.5 regression fix 2/2] efi/libstub/helper: Initialize pointer variables to zero for mixed mode
When running in EFI mixed mode (running a 64 bit kernel on 32 bit EFI firmware), we _must_ initialize any pointers which are returned by reference by an EFI call to NULL before making the EFI call.
In mixed mode pointers are 64 bit, but when running on a 32 bit firmware, EFI calls which return a pointer value by reference only fill the lower 32 bits of the passed pointer, leaving the upper 32 bits uninitialized unless we explicitly set them to 0 before the call.
We have had this bug in the efi-stub-helper.c file reading code for a while now, but this has likely not been noticed sofar because this code only gets triggered when LILO style file=... arguments are present on the kernel cmdline.
Cc: stable@vger.kernel.org Signed-off-by: Hans de Goede hdegoede@redhat.com --- drivers/firmware/efi/libstub/efi-stub-helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index e02579907f2e..6ca7d86743af 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -365,7 +365,7 @@ static efi_status_t efi_file_size(efi_system_table_t *sys_table_arg, void *__fh, u64 *file_sz) { efi_file_handle_t *h, *fh = __fh; - efi_file_info_t *info; + efi_file_info_t *info = NULL; efi_status_t status; efi_guid_t info_guid = EFI_FILE_INFO_ID; unsigned long info_sz; @@ -527,7 +527,7 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg, unsigned long *load_addr, unsigned long *load_size) { - struct file_info *files; + struct file_info *files = NULL; unsigned long file_addr; u64 file_size_total; efi_file_handle_t *fh = NULL;
On Thu, 12 Dec 2019 at 11:32, Hans de Goede hdegoede@redhat.com wrote:
When running in EFI mixed mode (running a 64 bit kernel on 32 bit EFI firmware), we _must_ initialize any pointers which are returned by reference by an EFI call to NULL before making the EFI call.
In mixed mode pointers are 64 bit, but when running on a 32 bit firmware, EFI calls which return a pointer value by reference only fill the lower 32 bits of the passed pointer, leaving the upper 32 bits uninitialized unless we explicitly set them to 0 before the call.
We have had this bug in the efi-stub-helper.c file reading code for a while now, but this has likely not been noticed sofar because this code only gets triggered when LILO style file=... arguments are present on the kernel cmdline.
Cc: stable@vger.kernel.org Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/firmware/efi/libstub/efi-stub-helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index e02579907f2e..6ca7d86743af 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -365,7 +365,7 @@ static efi_status_t efi_file_size(efi_system_table_t *sys_table_arg, void *__fh, u64 *file_sz) { efi_file_handle_t *h, *fh = __fh;
What about h? Doesn't it suffer from the same problem?
efi_file_info_t *info;
efi_file_info_t *info = NULL; efi_status_t status; efi_guid_t info_guid = EFI_FILE_INFO_ID; unsigned long info_sz;
And info_sz?
@@ -527,7 +527,7 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg, unsigned long *load_addr, unsigned long *load_size) {
struct file_info *files;
struct file_info *files = NULL; unsigned long file_addr; u64 file_size_total; efi_file_handle_t *fh = NULL;-- 2.23.0
Hi,
On 12-12-2019 12:29, Ard Biesheuvel wrote:
On Thu, 12 Dec 2019 at 11:32, Hans de Goede hdegoede@redhat.com wrote:
When running in EFI mixed mode (running a 64 bit kernel on 32 bit EFI firmware), we _must_ initialize any pointers which are returned by reference by an EFI call to NULL before making the EFI call.
In mixed mode pointers are 64 bit, but when running on a 32 bit firmware, EFI calls which return a pointer value by reference only fill the lower 32 bits of the passed pointer, leaving the upper 32 bits uninitialized unless we explicitly set them to 0 before the call.
We have had this bug in the efi-stub-helper.c file reading code for a while now, but this has likely not been noticed sofar because this code only gets triggered when LILO style file=... arguments are present on the kernel cmdline.
Cc: stable@vger.kernel.org Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/firmware/efi/libstub/efi-stub-helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index e02579907f2e..6ca7d86743af 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -365,7 +365,7 @@ static efi_status_t efi_file_size(efi_system_table_t *sys_table_arg, void *__fh, u64 *file_sz) { efi_file_handle_t *h, *fh = __fh;
What about h? Doesn't it suffer from the same problem?
efi_file_info_t *info;
efi_file_info_t *info = NULL; efi_status_t status; efi_guid_t info_guid = EFI_FILE_INFO_ID; unsigned long info_sz;And info_sz?
You are right in both cases. I only checked allocate_pool and locate_protocol callers as those are the usual suspects.
Shall I send a v2 of just this patch, or of the entire series, or are you going to fix this up?
Regards,
Hans
@@ -527,7 +527,7 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg, unsigned long *load_addr, unsigned long *load_size) {
struct file_info *files;
struct file_info *files = NULL; unsigned long file_addr; u64 file_size_total; efi_file_handle_t *fh = NULL;-- 2.23.0
Hi,
On 12-12-2019 12:29, Ard Biesheuvel wrote:
On Thu, 12 Dec 2019 at 11:32, Hans de Goede hdegoede@redhat.com wrote:
When running in EFI mixed mode (running a 64 bit kernel on 32 bit EFI firmware), we _must_ initialize any pointers which are returned by reference by an EFI call to NULL before making the EFI call.
In mixed mode pointers are 64 bit, but when running on a 32 bit firmware, EFI calls which return a pointer value by reference only fill the lower 32 bits of the passed pointer, leaving the upper 32 bits uninitialized unless we explicitly set them to 0 before the call.
We have had this bug in the efi-stub-helper.c file reading code for a while now, but this has likely not been noticed sofar because this code only gets triggered when LILO style file=... arguments are present on the kernel cmdline.
Cc: stable@vger.kernel.org Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/firmware/efi/libstub/efi-stub-helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index e02579907f2e..6ca7d86743af 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -365,7 +365,7 @@ static efi_status_t efi_file_size(efi_system_table_t *sys_table_arg, void *__fh, u64 *file_sz) { efi_file_handle_t *h, *fh = __fh;
What about h? Doesn't it suffer from the same problem?
efi_file_info_t *info;
efi_file_info_t *info = NULL; efi_status_t status; efi_guid_t info_guid = EFI_FILE_INFO_ID; unsigned long info_sz;And info_sz?
And "efi_file_io_interface_t *io" and "efi_file_handle_t *fh" in efi_open_volume().
I think that is all of them.
Regards,
Hans
On Thu, 12 Dec 2019 at 13:45, Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 12-12-2019 12:29, Ard Biesheuvel wrote:
On Thu, 12 Dec 2019 at 11:32, Hans de Goede hdegoede@redhat.com wrote:
When running in EFI mixed mode (running a 64 bit kernel on 32 bit EFI firmware), we _must_ initialize any pointers which are returned by reference by an EFI call to NULL before making the EFI call.
In mixed mode pointers are 64 bit, but when running on a 32 bit firmware, EFI calls which return a pointer value by reference only fill the lower 32 bits of the passed pointer, leaving the upper 32 bits uninitialized unless we explicitly set them to 0 before the call.
We have had this bug in the efi-stub-helper.c file reading code for a while now, but this has likely not been noticed sofar because this code only gets triggered when LILO style file=... arguments are present on the kernel cmdline.
Cc: stable@vger.kernel.org Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/firmware/efi/libstub/efi-stub-helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index e02579907f2e..6ca7d86743af 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -365,7 +365,7 @@ static efi_status_t efi_file_size(efi_system_table_t *sys_table_arg, void *__fh, u64 *file_sz) { efi_file_handle_t *h, *fh = __fh;
What about h? Doesn't it suffer from the same problem?
efi_file_info_t *info;
efi_file_info_t *info = NULL; efi_status_t status; efi_guid_t info_guid = EFI_FILE_INFO_ID; unsigned long info_sz;And info_sz?
And "efi_file_io_interface_t *io" and "efi_file_handle_t *fh" in efi_open_volume().
I think that is all of them.
OK.
I'll fix it up locally.
On Thu, 12 Dec 2019 at 15:02, Ard Biesheuvel ard.biesheuvel@linaro.org wrote:
On Thu, 12 Dec 2019 at 13:45, Hans de Goede hdegoede@redhat.com wrote:
Hi,
On 12-12-2019 12:29, Ard Biesheuvel wrote:
On Thu, 12 Dec 2019 at 11:32, Hans de Goede hdegoede@redhat.com wrote:
When running in EFI mixed mode (running a 64 bit kernel on 32 bit EFI firmware), we _must_ initialize any pointers which are returned by reference by an EFI call to NULL before making the EFI call.
In mixed mode pointers are 64 bit, but when running on a 32 bit firmware, EFI calls which return a pointer value by reference only fill the lower 32 bits of the passed pointer, leaving the upper 32 bits uninitialized unless we explicitly set them to 0 before the call.
We have had this bug in the efi-stub-helper.c file reading code for a while now, but this has likely not been noticed sofar because this code only gets triggered when LILO style file=... arguments are present on the kernel cmdline.
Cc: stable@vger.kernel.org Signed-off-by: Hans de Goede hdegoede@redhat.com
drivers/firmware/efi/libstub/efi-stub-helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index e02579907f2e..6ca7d86743af 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -365,7 +365,7 @@ static efi_status_t efi_file_size(efi_system_table_t *sys_table_arg, void *__fh, u64 *file_sz) { efi_file_handle_t *h, *fh = __fh;
What about h? Doesn't it suffer from the same problem?
efi_file_info_t *info;
efi_file_info_t *info = NULL; efi_status_t status; efi_guid_t info_guid = EFI_FILE_INFO_ID; unsigned long info_sz;And info_sz?
And "efi_file_io_interface_t *io" and "efi_file_handle_t *fh" in efi_open_volume().
I think that is all of them.
OK.
I'll fix it up locally.
Actually, I am going to drop this patch, and disable file loading entirely for mixed mode. Your findings here prove it is unlikely that it works on mixed mode systems today, and the efi_file_handle_t::open() prototype is actually incompatible with mixed mode, since it takes u64 arguments, which are marshalled incorrectly by the thunking code (each function arg gets a 32-bit stack slot for the 32-bit calling convention, which works fine for pointers and smaller types, but it breaks for u64 since they require two stack slots)
linux-stable-mirror@lists.linaro.org
-
Ard Biesheuvel -
Hans de Goede