From: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
To: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Cc: Dan Carpenter
<dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>,
"linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
<linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org>
Subject: Re: efi: Add 'capsule' update support
Date: Wed, 4 May 2016 14:28:50 +0200 [thread overview]
Message-ID: <CAKv+Gu9zw2ydSrRADz0RWa7N-9+bw4fqqPJua+3gC2i+RLNc4w@mail.gmail.com> (raw)
In-Reply-To: <20160504121241.GI2839-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
On 4 May 2016 at 14:12, Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Tue, 03 May, at 05:58:18PM, Ard Biesheuvel wrote:
>>
>> Well, I suppose we could simply allocate the pointer array and the
>> single member statically, i.e.,
>>
>> efi_capsule_header_t capsule1;
>> efi_capsule_header_t *capsule[] = { &capsule1 };
>>
>> That way, we can get rid of the heap allocation entirely, and we take
>> the address of the array, i.e., without the &
>
> This?
>
Yes, that looks correct to me, although I haven't tried to compile it.
Acked-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>
> From cc80265af2073e99627933d9a2192a175dddaca0 Mon Sep 17 00:00:00 2001
> From: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> Date: Wed, 4 May 2016 12:52:06 +0100
> Subject: [PATCH] efi/capsule: Move 'capsule' to the stack in
> efi_capsule_supported()
>
> Dan reports that passing the address of the pointer to the kmalloc()'d
> memory for 'capsule' is dangerous,
>
> "drivers/firmware/efi/capsule.c:109 efi_capsule_supported()
> warn: did you mean to pass the address of 'capsule'
>
> 108
> 109 status = efi.query_capsule_caps(&capsule, 1, &max_size, reset);
> ^^^^^^^^
> If we modify capsule inside this function call then at the end of the
> function we aren't freeing the original pointer that we allocated."
>
> Ard noted that we don't even need to call kmalloc() since the object
> we allocate isn't very big and doesn't need to persist after the
> function returns.
>
> Place 'capsule' on the stack instead.
>
> Suggested-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Reported-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Cc: Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org>
> Cc: Kweh Hock Leong <hock.leong.kweh-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Bryan O'Donoghue <pure.logic-SyKdqv6vbfZdzvEItQ6vdLNAH6kLmebB@public.gmane.org>
> Cc: joeyli <jlee-IBi9RG/b67k@public.gmane.org>
> Signed-off-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
> ---
> drivers/firmware/efi/capsule.c | 29 +++++++++++------------------
> 1 file changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
> index 4703dc9b8fbd..7593108f5402 100644
> --- a/drivers/firmware/efi/capsule.c
> +++ b/drivers/firmware/efi/capsule.c
> @@ -86,33 +86,26 @@ bool efi_capsule_pending(int *reset_type)
> */
> int efi_capsule_supported(efi_guid_t guid, u32 flags, size_t size, int *reset)
> {
> - efi_capsule_header_t *capsule;
> + efi_capsule_header_t capsule;
> + efi_capsule_header_t *cap_list[] = { &capsule };
> efi_status_t status;
> u64 max_size;
> - int rv = 0;
>
> if (flags & ~EFI_CAPSULE_SUPPORTED_FLAG_MASK)
> return -EINVAL;
>
> - capsule = kmalloc(sizeof(*capsule), GFP_KERNEL);
> - if (!capsule)
> - return -ENOMEM;
> -
> - capsule->headersize = capsule->imagesize = sizeof(*capsule);
> - memcpy(&capsule->guid, &guid, sizeof(efi_guid_t));
> - capsule->flags = flags;
> + capsule.headersize = capsule.imagesize = sizeof(capsule);
> + memcpy(&capsule.guid, &guid, sizeof(efi_guid_t));
> + capsule.flags = flags;
>
> - status = efi.query_capsule_caps(&capsule, 1, &max_size, reset);
> - if (status != EFI_SUCCESS) {
> - rv = efi_status_to_err(status);
> - goto out;
> - }
> + status = efi.query_capsule_caps(cap_list, 1, &max_size, reset);
> + if (status != EFI_SUCCESS)
> + return efi_status_to_err(status);
>
> if (size > max_size)
> - rv = -ENOSPC;
> -out:
> - kfree(capsule);
> - return rv;
> + return -ENOSPC;
> +
> + return 0;
> }
> EXPORT_SYMBOL_GPL(efi_capsule_supported);
>
> --
> 2.7.3
>
prev parent reply other threads:[~2016-05-04 12:28 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-02 18:25 efi: Add 'capsule' update support Dan Carpenter
2016-05-03 14:50 ` Matt Fleming
[not found] ` <20160503145050.GY2839-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
2016-05-03 15:58 ` Ard Biesheuvel
[not found] ` <CAKv+Gu-CBymL85xJnmC9d9nuUOuMU3kBAONWtj1-_ck4hEzxiQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2016-05-04 12:12 ` Matt Fleming
[not found] ` <20160504121241.GI2839-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
2016-05-04 12:28 ` Ard Biesheuvel [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKv+Gu9zw2ydSrRADz0RWa7N-9+bw4fqqPJua+3gC2i+RLNc4w@mail.gmail.com \
--to=ard.biesheuvel-qsej5fyqhm4dnm+yrofe0a@public.gmane.org \
--cc=bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org \
--cc=dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org \
--cc=linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).