[AMD Official Use Only - General] > -----Original Message----- > From: Ghannam, Yazen <Yazen.Ghannam@xxxxxxx> > Sent: Friday, August 26, 2022 11:20 AM > To: Russell, Kent <Kent.Russell@xxxxxxx> > Cc: Lazar, Lijo <Lijo.Lazar@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH] drm/amdgpu: Handle potential NULL pointer dereference > > On Thu, Aug 25, 2022 at 08:54:46AM -0400, Russell, Kent wrote: > > [AMD Official Use Only - General] > > > > It does indeed short-circuit on || (If the left side of an || statement is not 0, it > doesn't evaluate the right side and returns true). So we can ignore this patch, > since checking for each individual field on the 2nd term is probably overkill. We > were still investigating what got passed in and why it wasn't valid, so I'll drop this > for now. Thanks Lijo! > > Right, the "||" will short-circuit, but not the "&&". > > ... > > > > + if (!m) > > > + return NOTIFY_DONE; > > > + > > > /* > > > * If the error was generated in UMC_V2, which belongs to GPU UMCs, > > > * and error occurred in DramECC (Extended error code = 0) then only > > > * process the error, else bail out. > > > */ > > > - if (!m || !((smca_get_bank_type(m->extcpu, m->bank) == > SMCA_UMC_V2) && > > > + if (!((smca_get_bank_type(m->extcpu, m->bank) == SMCA_UMC_V2) && > > > (XEC(m->status, 0x3f) == 0x0))) > > If m is NULL, then we get (!m || "Don't care") = true. We avoid the second > operand. > > But then we have (true && "Something with 'm->' in it"). The second operand in > the && needs to be checked, and this will cause a NULL pointer dereference if > m is NULL. > > What do you guys think? > Since the 2nd field is in parentheses, it would be whatever = (smca_get_bank_type && XEC) if (!m || !(whatever)) In that case we should still jump out at !m, since the 2nd term is completely contained in that !((...)) clump. At least that's how I understand it after Lijo's original comments. Kent > Thanks, > Yazen
