| Message ID | 20180131214643.11327-2-manoj.iyer@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | efi/capsule-loader: Reinstate virtual capsule mapping | expand |
On 31.01.2018 22:46, Manoj Iyer wrote: > From: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > Commit: > > 82c3768b8d68 ("efi/capsule-loader: Use a cached copy of the capsule header") > > ... refactored the capsule loading code that maps the capsule header, > to avoid having to map it several times. > > However, as it turns out, the vmap() call we ended up removing did not > just map the header, but the entire capsule image, and dropping this > virtual mapping breaks capsules that are processed by the firmware > immediately (i.e., without a reboot). > > Unfortunately, that change was part of a larger refactor that allowed > a quirk to be implemented for Quark, which has a non-standard memory > layout for capsules, and we have slightly painted ourselves into a > corner by allowing quirk code to mangle the capsule header and memory > layout. > > So we need to fix this without breaking Quark. Fortunately, Quark does > not appear to care about the virtual mapping, and so we can simply > do a partial revert of commit: > > 2a457fb31df6 ("efi/capsule-loader: Use page addresses rather than struct page pointers") > > ... and create a vmap() mapping of the entire capsule (including header) > based on the reinstated struct page array, unless running on Quark, in > which case we pass the capsule header copy as before. > > BugLink: https://launchpad.net/bugs/1746019 > > Reported-by: Ge Song <ge.song@hxt-semitech.com> > Tested-by: Bryan O'Donoghue <pure.logic@nexus-software.ie> > Tested-by: Ge Song <ge.song@hxt-semitech.com> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Cc: <stable@vger.kernel.org> > Cc: Dave Young <dyoung@redhat.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Matt Fleming <matt@codeblueprint.co.uk> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: linux-efi@vger.kernel.org > Fixes: 82c3768b8d68 ("efi/capsule-loader: Use a cached copy of the capsule header") > Link: http://lkml.kernel.org/r/20180102172110.17018-3-ard.biesheuvel@linaro.org > Signed-off-by: Ingo Molnar <mingo@kernel.org> > (cherry picked from commit f24c4d478013d82bd1b943df566fff3561d52864) > Signed-off-by: Manoj Iyer <manoj.iyer@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- > arch/x86/platform/efi/quirks.c | 13 +++++++++- > drivers/firmware/efi/capsule-loader.c | 45 ++++++++++++++++++++++++++++------- > include/linux/efi.h | 4 +++- > 3 files changed, 52 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c > index 8a99a2e96537..5b513ccffde4 100644 > --- a/arch/x86/platform/efi/quirks.c > +++ b/arch/x86/platform/efi/quirks.c > @@ -592,7 +592,18 @@ static int qrk_capsule_setup_info(struct capsule_info *cap_info, void **pkbuff, > /* > * Update the first page pointer to skip over the CSH header. > */ > - cap_info->pages[0] += csh->headersize; > + cap_info->phys[0] += csh->headersize; > + > + /* > + * cap_info->capsule should point at a virtual mapping of the entire > + * capsule, starting at the capsule header. Our image has the Quark > + * security header prepended, so we cannot rely on the default vmap() > + * mapping created by the generic capsule code. > + * Given that the Quark firmware does not appear to care about the > + * virtual mapping, let's just point cap_info->capsule at our copy > + * of the capsule header. > + */ > + cap_info->capsule = &cap_info->header; > > return 1; > } > diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c > index ec8ac5c4dd84..055e2e8f985a 100644 > --- a/drivers/firmware/efi/capsule-loader.c > +++ b/drivers/firmware/efi/capsule-loader.c > @@ -20,10 +20,6 @@ > > #define NO_FURTHER_WRITE_ACTION -1 > > -#ifndef phys_to_page > -#define phys_to_page(x) pfn_to_page((x) >> PAGE_SHIFT) > -#endif > - > /** > * efi_free_all_buff_pages - free all previous allocated buffer pages > * @cap_info: pointer to current instance of capsule_info structure > @@ -35,7 +31,7 @@ > static void efi_free_all_buff_pages(struct capsule_info *cap_info) > { > while (cap_info->index > 0) > - __free_page(phys_to_page(cap_info->pages[--cap_info->index])); > + __free_page(cap_info->pages[--cap_info->index]); > > cap_info->index = NO_FURTHER_WRITE_ACTION; > } > @@ -71,6 +67,14 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info) > > cap_info->pages = temp_page; > > + temp_page = krealloc(cap_info->phys, > + pages_needed * sizeof(phys_addr_t *), > + GFP_KERNEL | __GFP_ZERO); > + if (!temp_page) > + return -ENOMEM; > + > + cap_info->phys = temp_page; > + > return 0; > } > > @@ -105,9 +109,24 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff, > **/ > static ssize_t efi_capsule_submit_update(struct capsule_info *cap_info) > { > + bool do_vunmap = false; > int ret; > > - ret = efi_capsule_update(&cap_info->header, cap_info->pages); > + /* > + * cap_info->capsule may have been assigned already by a quirk > + * handler, so only overwrite it if it is NULL > + */ > + if (!cap_info->capsule) { > + cap_info->capsule = vmap(cap_info->pages, cap_info->index, > + VM_MAP, PAGE_KERNEL); > + if (!cap_info->capsule) > + return -ENOMEM; > + do_vunmap = true; > + } > + > + ret = efi_capsule_update(cap_info->capsule, cap_info->phys); > + if (do_vunmap) > + vunmap(cap_info->capsule); > if (ret) { > pr_err("capsule update failed\n"); > return ret; > @@ -165,10 +184,12 @@ static ssize_t efi_capsule_write(struct file *file, const char __user *buff, > goto failed; > } > > - cap_info->pages[cap_info->index++] = page_to_phys(page); > + cap_info->pages[cap_info->index] = page; > + cap_info->phys[cap_info->index] = page_to_phys(page); > cap_info->page_bytes_remain = PAGE_SIZE; > + cap_info->index++; > } else { > - page = phys_to_page(cap_info->pages[cap_info->index - 1]); > + page = cap_info->pages[cap_info->index - 1]; > } > > kbuff = kmap(page); > @@ -252,6 +273,7 @@ static int efi_capsule_release(struct inode *inode, struct file *file) > struct capsule_info *cap_info = file->private_data; > > kfree(cap_info->pages); > + kfree(cap_info->phys); > kfree(file->private_data); > file->private_data = NULL; > return 0; > @@ -281,6 +303,13 @@ static int efi_capsule_open(struct inode *inode, struct file *file) > return -ENOMEM; > } > > + cap_info->phys = kzalloc(sizeof(void *), GFP_KERNEL); > + if (!cap_info->phys) { > + kfree(cap_info->pages); > + kfree(cap_info); > + return -ENOMEM; > + } > + > file->private_data = cap_info; > > return 0; > diff --git a/include/linux/efi.h b/include/linux/efi.h > index 399f17a9f118..247eae203f97 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -139,11 +139,13 @@ struct efi_boot_memmap { > > struct capsule_info { > efi_capsule_header_t header; > + efi_capsule_header_t *capsule; > int reset_type; > long index; > size_t count; > size_t total_size; > - phys_addr_t *pages; > + struct page **pages; > + phys_addr_t *phys; > size_t page_bytes_remain; > }; > >
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c index 8a99a2e96537..5b513ccffde4 100644 --- a/arch/x86/platform/efi/quirks.c +++ b/arch/x86/platform/efi/quirks.c @@ -592,7 +592,18 @@ static int qrk_capsule_setup_info(struct capsule_info *cap_info, void **pkbuff, /* * Update the first page pointer to skip over the CSH header. */ - cap_info->pages[0] += csh->headersize; + cap_info->phys[0] += csh->headersize; + + /* + * cap_info->capsule should point at a virtual mapping of the entire + * capsule, starting at the capsule header. Our image has the Quark + * security header prepended, so we cannot rely on the default vmap() + * mapping created by the generic capsule code. + * Given that the Quark firmware does not appear to care about the + * virtual mapping, let's just point cap_info->capsule at our copy + * of the capsule header. + */ + cap_info->capsule = &cap_info->header; return 1; } diff --git a/drivers/firmware/efi/capsule-loader.c b/drivers/firmware/efi/capsule-loader.c index ec8ac5c4dd84..055e2e8f985a 100644 --- a/drivers/firmware/efi/capsule-loader.c +++ b/drivers/firmware/efi/capsule-loader.c @@ -20,10 +20,6 @@ #define NO_FURTHER_WRITE_ACTION -1 -#ifndef phys_to_page -#define phys_to_page(x) pfn_to_page((x) >> PAGE_SHIFT) -#endif - /** * efi_free_all_buff_pages - free all previous allocated buffer pages * @cap_info: pointer to current instance of capsule_info structure @@ -35,7 +31,7 @@ static void efi_free_all_buff_pages(struct capsule_info *cap_info) { while (cap_info->index > 0) - __free_page(phys_to_page(cap_info->pages[--cap_info->index])); + __free_page(cap_info->pages[--cap_info->index]); cap_info->index = NO_FURTHER_WRITE_ACTION; } @@ -71,6 +67,14 @@ int __efi_capsule_setup_info(struct capsule_info *cap_info) cap_info->pages = temp_page; + temp_page = krealloc(cap_info->phys, + pages_needed * sizeof(phys_addr_t *), + GFP_KERNEL | __GFP_ZERO); + if (!temp_page) + return -ENOMEM; + + cap_info->phys = temp_page; + return 0; } @@ -105,9 +109,24 @@ int __weak efi_capsule_setup_info(struct capsule_info *cap_info, void *kbuff, **/ static ssize_t efi_capsule_submit_update(struct capsule_info *cap_info) { + bool do_vunmap = false; int ret; - ret = efi_capsule_update(&cap_info->header, cap_info->pages); + /* + * cap_info->capsule may have been assigned already by a quirk + * handler, so only overwrite it if it is NULL + */ + if (!cap_info->capsule) { + cap_info->capsule = vmap(cap_info->pages, cap_info->index, + VM_MAP, PAGE_KERNEL); + if (!cap_info->capsule) + return -ENOMEM; + do_vunmap = true; + } + + ret = efi_capsule_update(cap_info->capsule, cap_info->phys); + if (do_vunmap) + vunmap(cap_info->capsule); if (ret) { pr_err("capsule update failed\n"); return ret; @@ -165,10 +184,12 @@ static ssize_t efi_capsule_write(struct file *file, const char __user *buff, goto failed; } - cap_info->pages[cap_info->index++] = page_to_phys(page); + cap_info->pages[cap_info->index] = page; + cap_info->phys[cap_info->index] = page_to_phys(page); cap_info->page_bytes_remain = PAGE_SIZE; + cap_info->index++; } else { - page = phys_to_page(cap_info->pages[cap_info->index - 1]); + page = cap_info->pages[cap_info->index - 1]; } kbuff = kmap(page); @@ -252,6 +273,7 @@ static int efi_capsule_release(struct inode *inode, struct file *file) struct capsule_info *cap_info = file->private_data; kfree(cap_info->pages); + kfree(cap_info->phys); kfree(file->private_data); file->private_data = NULL; return 0; @@ -281,6 +303,13 @@ static int efi_capsule_open(struct inode *inode, struct file *file) return -ENOMEM; } + cap_info->phys = kzalloc(sizeof(void *), GFP_KERNEL); + if (!cap_info->phys) { + kfree(cap_info->pages); + kfree(cap_info); + return -ENOMEM; + } + file->private_data = cap_info; return 0; diff --git a/include/linux/efi.h b/include/linux/efi.h index 399f17a9f118..247eae203f97 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -139,11 +139,13 @@ struct efi_boot_memmap { struct capsule_info { efi_capsule_header_t header; + efi_capsule_header_t *capsule; int reset_type; long index; size_t count; size_t total_size; - phys_addr_t *pages; + struct page **pages; + phys_addr_t *phys; size_t page_bytes_remain; };