| Message ID | 1574041813-35408-1-git-send-email-pannengyuan@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | ppc/spapr_events: fix potential NULL pointer dereference in rtas_event_log_dequeue | expand |
On Mon, Nov 18, 2019 at 09:50:13AM +0800, pannengyuan@huawei.com wrote: > From: PanNengyuan <pannengyuan@huawei.com> > > source is being dereferenced before it is null checked, hence there is a > potential null pointer dereference. > > This fixes: > 360 > CID 68911917: (NULL_RETURNS) > 361. dereference: Dereferencing "source", which is known to be > "NULL". > 361 if (source->mask & event_mask) { > 362 break; > 363 } > > Reported-by: Euler Robot <euler.robot@huawei.com> > Signed-off-by: PanNengyuan <pannengyuan@huawei.com> I don't think this is the right solution. The only events we ever generated are LOG_TYPE_EPOW and LOG_TYPE_HOTPLUG, so in fact source should never be NULL. I think the correct way to satisfy Coverity here is to have rtas_event_log_to_source() do an assert(), rather than returning NULL for other event types. > --- > hw/ppc/spapr_events.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > index 0e4c195..febd2ef 100644 > --- a/hw/ppc/spapr_events.c > +++ b/hw/ppc/spapr_events.c > @@ -358,7 +358,7 @@ static SpaprEventLogEntry *rtas_event_log_dequeue(SpaprMachineState *spapr, > rtas_event_log_to_source(spapr, > spapr_event_log_entry_type(entry)); > > - if (source->mask & event_mask) { > + if (source && (source->mask & event_mask)) { > break; > } > }
Thanks for you reply. I think you are right, I will send a new version later. On 2019/11/19 10:50, David Gibson wrote: > On Mon, Nov 18, 2019 at 09:50:13AM +0800, pannengyuan@huawei.com wrote: >> From: PanNengyuan <pannengyuan@huawei.com> >> >> source is being dereferenced before it is null checked, hence there is a >> potential null pointer dereference. >> >> This fixes: >> 360 >> CID 68911917: (NULL_RETURNS) >> 361. dereference: Dereferencing "source", which is known to be >> "NULL". >> 361 if (source->mask & event_mask) { >> 362 break; >> 363 } >> >> Reported-by: Euler Robot <euler.robot@huawei.com> >> Signed-off-by: PanNengyuan <pannengyuan@huawei.com> > > I don't think this is the right solution. The only events we ever > generated are LOG_TYPE_EPOW and LOG_TYPE_HOTPLUG, so in fact source > should never be NULL. > > I think the correct way to satisfy Coverity here is to have > rtas_event_log_to_source() do an assert(), rather than returning NULL > for other event types. > >> --- >> hw/ppc/spapr_events.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c >> index 0e4c195..febd2ef 100644 >> --- a/hw/ppc/spapr_events.c >> +++ b/hw/ppc/spapr_events.c >> @@ -358,7 +358,7 @@ static SpaprEventLogEntry *rtas_event_log_dequeue(SpaprMachineState *spapr, >> rtas_event_log_to_source(spapr, >> spapr_event_log_entry_type(entry)); >> >> - if (source->mask & event_mask) { >> + if (source && (source->mask & event_mask)) { >> break; >> } >> } >
diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c index 0e4c195..febd2ef 100644 --- a/hw/ppc/spapr_events.c +++ b/hw/ppc/spapr_events.c @@ -358,7 +358,7 @@ static SpaprEventLogEntry *rtas_event_log_dequeue(SpaprMachineState *spapr, rtas_event_log_to_source(spapr, spapr_event_log_entry_type(entry)); - if (source->mask & event_mask) { + if (source && (source->mask & event_mask)) { break; } }