diff mbox series

[ovs-dev,v1,1/1] ofproto: Fix NULL deref reported by Coverity.

Message ID SY8P282MB4580385277236FE947D23E94CD3E2@SY8P282MB4580.AUSP282.PROD.OUTLOOK.COM
State Changes Requested
Delegated to: Simon Horman
Headers show
Series ofproto: Fix NULL deref reported by Coverity. | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

miter April 2, 2024, 2:08 p.m. UTC 
From: miter <miterv@outlook.com>

Ofproto_class_find__() may return NULL, and dereference it to cause
segfault.

Tested-by: Zhang YuHuang <zhangyuhuang@ruijie.com.cn>
Signed-off-by: Lin Huang <linhuang@ruijie.com.cn>
---
 ofproto/ofproto.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Aaron Conole April 2, 2024, 5:04 p.m. UTC | #1 
miterv@outlook.com writes:

> From: miter <miterv@outlook.com>
>
> Ofproto_class_find__() may return NULL, and dereference it to cause
> segfault.
>
> Tested-by: Zhang YuHuang <zhangyuhuang@ruijie.com.cn>
> Signed-off-by: Lin Huang <linhuang@ruijie.com.cn>
> ---

I guess that type_run and type_wait aren't flagged this way because the
only users walk the ofproto types list explicitly (so we don't get into
this situation)?

>  ofproto/ofproto.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 122a06f30..21c6a1d82 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -800,7 +800,7 @@ ofproto_type_set_config(const char *datapath_type, const struct smap *cfg)
>      datapath_type = ofproto_normalize_type(datapath_type);
>      class = ofproto_class_find__(datapath_type);
>  
> -    if (class->type_set_config) {
> +    if (class && class->type_set_config) {

This change looks good to me.  Have you a related issue or is it just
preventative?

>          class->type_set_config(datapath_type, cfg);
>      }
>  }
Ilya Maximets April 2, 2024, 5:26 p.m. UTC | #2 
On 4/2/24 19:04, Aaron Conole wrote:
> miterv@outlook.com writes:
> 
>> From: miter <miterv@outlook.com>

Please, adjust the 'From' to match the Sign-off tag.

>>
>> Ofproto_class_find__() may return NULL, and dereference it to cause
>> segfault.
>>
>> Tested-by: Zhang YuHuang <zhangyuhuang@ruijie.com.cn>
>> Signed-off-by: Lin Huang <linhuang@ruijie.com.cn>
>> ---
> 
> I guess that type_run and type_wait aren't flagged this way because the
> only users walk the ofproto types list explicitly (so we don't get into
> this situation)?
> 
>>  ofproto/ofproto.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> index 122a06f30..21c6a1d82 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -800,7 +800,7 @@ ofproto_type_set_config(const char *datapath_type, const struct smap *cfg)
>>      datapath_type = ofproto_normalize_type(datapath_type);
>>      class = ofproto_class_find__(datapath_type);
>>  
>> -    if (class->type_set_config) {
>> +    if (class && class->type_set_config) {
> 
> This change looks good to me.  Have you a related issue or is it just
> preventative?

Hmm.  This function is only called with pre-registered types, so
this condition should not be actually possible.

The change itself is fine, but, I think, the patch name and the commit
message should be changed to indicate that this is not a real issue
and the change is made just to avoid false-positive Coverity report.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 122a06f30..21c6a1d82 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -800,7 +800,7 @@  ofproto_type_set_config(const char *datapath_type, const struct smap *cfg)
     datapath_type = ofproto_normalize_type(datapath_type);
     class = ofproto_class_find__(datapath_type);
 
-    if (class->type_set_config) {
+    if (class && class->type_set_config) {
         class->type_set_config(datapath_type, cfg);
     }
 }