[Skiboot] [PATCH] nvram: Fix a possible NULL pointer de-ref in nvram_query_eq()

[Oliver]

On Wed, Sep 12, 2018 at 3:51 PM, Vaibhav Jain <vaibhav at linux.ibm.com> wrote:
> Thanks for looking into this patch Oliver.
>
> Oliver <oohall at gmail.com> writes:
>
>> This isn't a valid use of the API. nvram_query_eq() is just a helper
>> for the common case where there's only one valid value for that key
>> (e.g. fast-reset=0). If you're doing anything more complex you should
>> be using nvram_query() to retrieve the value (which is NULL if the key
>> doesn't exist) and checking each case manually.
>
> I looked at existing code and the commit description and inferred that:
>
> Since:
>         nvram_query_eq(key,value) <=> (nvram_query(key) == value)
> hence,
>         nvram_query_eq(key,NULL) <=> (nvram_query(key) == NULL)
>
> For keys which are essentially boolean flags this makes it easy to
> check if a given key is not defined in the nvram and. So, I dont
> understand why it should not be a valid use of the API.

When I wrote that I briefly considered doing this, but I didn't becase
"!nvram_query(key)" does the same job. It's also:

a) shorter,
b) clearly indicates that the value of the key is irrelevant, and
c) doesn't force the reader to figure out if passing a NULL is fine
and not a bug

> The proposed change makes the function more general and protects it
> against an invalid NULL arg.
>
>>> Signed-off-by: Vaibhav Jain <vaibhav at linux.ibm.com>
>>> ---
>>>  core/nvram-format.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/core/nvram-format.c b/core/nvram-format.c
>>> index 42c5cbbb..c52b443a 100644
>>> --- a/core/nvram-format.c
>>> +++ b/core/nvram-format.c
>>> @@ -282,8 +282,8 @@ bool nvram_query_eq(const char *key, const char *value)
>>>  {
>>>         const char *s = nvram_query(key);
>>>
>>> -       if (!s)
>>> -               return false;
>>> +       if (s == NULL || value == NULL)
>>> +               return s == value;
>>
>> Put an assert(value) before the strcmp and leave it at that. Maybe add
>> comment about the intended usage of nvram_query_eq()
>
> --
> Vaibhav Jain <vaibhav at linux.vnet.ibm.com>
> Linux Technology Center, IBM India Pvt. Ltd.
>