| Message ID | 20180908065122.9504-1-vaibhav@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | nvram: Fix a possible NULL pointer de-ref in nvram_query_eq() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | master/apply_patch Successfully applied |
| snowpatch_ozlabs/make_check | success | Test make_check on branch master |
On Sat, Sep 8, 2018 at 4:51 PM, Vaibhav Jain <vaibhav@linux.ibm.com> wrote: > A fault will occur if 'value == NULL' is passed to nvram_query_eq() to > check if a given key doesn't exists in nvram partition. This patch > fixes this issue by ensuring that a NULL value for argument 'value' > never reaches the call to strcmp(). 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. > Signed-off-by: Vaibhav Jain <vaibhav@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() > > return !strcmp(s, value); > } > -- > 2.17.1 Also, just FYI Cyril isn't working at IBM any more.
Thanks for looking into this patch Oliver. Oliver <oohall@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. The proposed change makes the function more general and protects it against an invalid NULL arg. > >> Signed-off-by: Vaibhav Jain <vaibhav@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()
On Wed, Sep 12, 2018 at 3:51 PM, Vaibhav Jain <vaibhav@linux.ibm.com> wrote: > Thanks for looking into this patch Oliver. > > Oliver <oohall@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@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@linux.vnet.ibm.com> > Linux Technology Center, IBM India Pvt. Ltd. >
Oliver <oohall@gmail.com> writes: > 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 Agree, to the points mentioned above. But how does they go against the proposed patch ? IMHO we are just adding one more way to do the same thing and in the process also making the function more robust for a boundary case. To me in this context, trigerring an assert or crashing due to a fault will have the same impact.
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; return !strcmp(s, value); }
A fault will occur if 'value == NULL' is passed to nvram_query_eq() to check if a given key doesn't exists in nvram partition. This patch fixes this issue by ensuring that a NULL value for argument 'value' never reaches the call to strcmp(). Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com> --- core/nvram-format.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)