Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220331221030.889718-1-jakobkoschel@gmail.com>
 <CAMj1kXE2r4xrtFc+=OJfzutZzTtaUoFtW=f7y9+us9h+xGVEnA@mail.gmail.com> <9142505E-720F-401E-AD48-BA9D0880EDD1@gmail.com>
In-Reply-To: <9142505E-720F-401E-AD48-BA9D0880EDD1@gmail.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Thu, 14 Apr 2022 00:09:07 +0200
Message-ID: <CAMj1kXFqG0n1KVPsTYo2a-qS3zb40itNmxy9gGy2A===qdPYxg@mail.gmail.com>
Subject: Re: [PATCH 1/2] efi: remove use of list iterator variable after loop
To:     Jakob Koschel <jakobkoschel@gmail.com>,
        Kees Cook <keescook@chromium.org>
Cc:     Matthew Garrett <mjg59@srcf.ucam.org>,
        Peter Jones <pjones@redhat.com>,
        linux-efi <linux-efi@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Mike Rapoport <rppt@kernel.org>,
        Brian Johannesmeyer <bjohannesmeyer@gmail.com>,
        Cristiano Giuffrida <c.giuffrida@vu.nl>,
        "Bos, H.J." <h.j.bos@vu.nl>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

(cc Kees for pstore)

On Wed, 13 Apr 2022 at 20:08, Jakob Koschel <jakobkoschel@gmail.com> wrote:
>
>
>
> > On 13. Apr 2022, at 19:05, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Fri, 1 Apr 2022 at 00:11, Jakob Koschel <jakobkoschel@gmail.com> wrote:
> >>
> >> In preparation to limiting the scope of a list iterator to the list
> >> traversal loop, use a dedicated pointer to iterate through the list [1].
> >>
> >> In the current state the list_for_each_entry() is guaranteed to
> >> hit a break or goto in order to work properly. If the list iterator
> >> executes completely or the list is empty the iterator variable contains
> >> a type confused bogus value infered from the head of the list.
> >>
> >> With this patch the variable used past the list iterator is only set
> >> if the list exists early and is NULL otherwise. It should therefore
> >> be safe to just set *prev = NULL (as it was before).
> >>
> >
> > This generic boilerplate is fine to include, but it would help if you
> > could point out why repainting the current logic with your new brush
> > is appropriate here.
>
> This makes sense, I can see that the commit message should be improved here.
>
> >
> > In this particular case, I wonder whether updating *prev makes sense
> > to begin with if we are returning an error, and if we fix that, the
> > issue disappears as well.
>
> Actually I'm rethinking this now. The only use of 'prev' that I can see is
> in efi_pstore_erase_name(). It only uses it if found != 0
> which would mean err != 0 in __efivar_entry_iter().
>
> This would allow massively simplifying the entire function.
> The valid case is updating *prev when there is an "error" as far as I can tell.
>

OK, so in summary, the only user of that code that bothers to pass a
value for prev abuses it to implement its own version of
efivar_entry_find(), and so if we fix that caller, we can drop the
'prev' argument from this function altogether.


> I've sketched up a rewritten function that should hopefully be more clear and
> archive the same goal, I'm curious what you think:
>
>
>         int __efivar_entry_iter(int (*func)(struct efivar_entry *, void *),
>                                 struct list_head *head, void *data,
>                                 struct efivar_entry **prev)
>         {
>                 struct efivar_entry *entry, *n;
>                 int err = 0;
>
>                 /* If prev is set and *prev != NULL start iterating from there */
>                 if (prev)
>                         entry = list_prepare_entry(*prev, head, list);
>                 /* Otherwise start at the beginning */
>                 else
>                         entry = list_entry(head, typeof(*entry), list);
>                 list_for_each_entry_safe_continue(entry, n, head, list) {
>                         err = func(entry, data);
>                         if (err && prev)
>                                 *prev = entry;
>                         if (err)
>                                 return err;
>                 }
>
>                 return 0;
>         }
>

Thanks for this. I'll have a stab myself at fixing the EFI pstore
code, and hopefully we can clean up __efivar_entry_iter() as I
suggested.