linux-kernel - Re: [PATCH 2/2] of: Add array read functions with min/max size limits

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 8 Sep 2016 16:46:01 +0100
From:   Richard Fitzgerald <rf@...nsource.wolfsonmicro.com>
To:     Rob Herring <robh+dt@...nel.org>
CC:     Frank Rowand <frowand.list@...il.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        <patches@...nsource.wolfsonmicro.com>
Subject: Re: [PATCH 2/2] of: Add array read functions with min/max size
 limits

On Thu, 2016-09-08 at 10:38 -0500, Rob Herring wrote:
> On Thu, Sep 8, 2016 at 10:34 AM, Richard Fitzgerald
> <rf@...nsource.wolfsonmicro.com> wrote:
> > On Thu, 2016-09-08 at 09:46 -0500, Rob Herring wrote:
> >> On Tue, Sep 6, 2016 at 10:02 AM, Richard Fitzgerald
> >> <rf@...nsource.wolfsonmicro.com> wrote:
> >> > Add a new set of array reading functions that take a minimum and
> >> > maximum size limit and will fail if the property size is not within
> >> > the size limits. This makes it more convenient for drivers that
> >> > use variable-size DT arrays which must be bounded at both ends -
> >> > data must be at least N entries but must not overflow the array
> >> > it is being copied into. It is also more efficient than making this
> >> > functionality out of existing public functions and avoids duplication.
> >> >
> >> > The existing array functions have been left in the API, since there
> >> > are a very large number of clients of those functions and their
> >> > existing functionality is still useful. This avoids turning a small
> >> > API improvement into a major kernel rework.
> 
> [...]
> 
> >> > @@ -1229,21 +1270,53 @@ EXPORT_SYMBOL_GPL(of_property_read_u32_index);
> >> >  int of_property_read_u8_array(const struct device_node *np,
> >> >                         const char *propname, u8 *out_values, size_t sz)
> >> >  {
> >> > -       const u8 *val = of_find_property_value_of_size(np, propname,
> >> > -                                               (sz * sizeof(*out_values)),
> >> > -                                               0,
> >> > -                                               NULL);
> >> > -
> >> > -       if (IS_ERR(val))
> >> > -               return PTR_ERR(val);
> >> > -
> >> > -       while (sz--)
> >> > -               *out_values++ = *val++;
> >> > -       return 0;
> >> > +       return of_property_read_variable_u8_array(np, propname, out_values,
> >> > +                                                 sz, 0);
> >>
> >> This should be min and max both set to sz.
> >
> > Passing 0 as max preserves the existing behaviour of these functions of
> > only requiring the array to be at least sz long, but not caring if it's
> > longer.
> 
> Yes, I was just writing to say that after reading patch 1 more carefully.
> 

Although at the same time I was realizing that actually my code is
broken for that and somehow I missed validating it in my testing.
The new functions validate the min and max against the DT entry size and
copy the actual number of elements. The old functions effectively
validate only the min and return that number of elements.

I'm just considering whether it's worth fixing my new functions to keep
the intention that max=0 duplicates the old behaviour, or not bother and
just keep the old functions.

> Rob