[Skiboot] [PATCH] hdata: Prevent NULL dereference on duplicate entries in TMPREL section

[Mahesh J Salgaonkar]

On 2025-03-22 09:11:33 Sat, Madhavan Srinivasan wrote:
> 
> 
> On 3/21/25 10:40 PM, Mahesh Salgaonkar wrote:
> > Currently if you encounter duplicate entries in TMPREL section while
> > parsing HDAT, opal crashes with bellow back trace:
> > 
> > [  119.205498180,3] DT: dt_attach_root failed, duplicate ibm,cvc-service at 40
> > [  119.206975658,3] ***********************************************
> > [  119.208669044,3] Fatal MCE at 000000003003729c
> > .dt_find_property+0x30  MSR 9000000000001002
> > [  119.210355268,3] Cause: unknown error
> > [  119.211273270,3] CFAR : 0000000030037288 MSR  : 9000000000001002
> > [  119.212502638,3] SRR0 : 000000003003729c SRR1 : 9000000000001002
> > [  119.214037362,3] HSRR0: 0000000030020024 HSRR1: 9000000000001000
> > [  119.215266730,3] DSISR: 40000000         DAR  : a600607d01006b79
> > [...]
> > CPU 0008 Backtrace:
> >  S: 0000000031c53980 R: 0000000030026b0c   .__memalign+0x58
> >  S: 0000000031c53a10 R: 0000000030037378   .new_property+0xb0
> >  S: 0000000031c53aa0 R: 0000000030037778   .__dt_add_property_strings+0x58
> >  S: 0000000031c53b40 R: 000000003010bf74   .node_stb_parse+0x414
> >  S: 0000000031c53c30 R: 0000000030102ee4   .parse_hdat+0x20cc
> >  S: 0000000031c53e30 R: 0000000030022c04   .main_cpu_entry+0x1d0
> >  S: 0000000031c53f00 R: 000000003000321c   go_primary+0x10c
> >  --- OPAL boot ---
> > 
> > fix the null pointer deref and proceed with warning message instead of
> > crashing. Also add debug prints to display all entries.
> > 
> > Signed-off-by: Mahesh Salgaonkar <mahesh at linux.ibm.com>
> > ---
> >  hdata/tpmrel.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hdata/tpmrel.c b/hdata/tpmrel.c
> > index c70791839..7f9dfa296 100644
> > --- a/hdata/tpmrel.c
> > +++ b/hdata/tpmrel.c
> > @@ -181,7 +181,7 @@ static void tpmrel_cvc_init(struct HDIF_common_hdr *hdif_hdr)
> >  
> >  	for (i = 0; i < count; i++) {
> >  		const struct hash_and_verification *hv;
> > -		uint32_t type, offset, version;
> > +		uint32_t type, offset, version, dbob_id;
> >  		const char *compat;
> >  
> >  		hv = HDIF_get_iarray_item(hdif_hdr,
> > @@ -190,6 +190,9 @@ static void tpmrel_cvc_init(struct HDIF_common_hdr *hdif_hdr)
> >  		type = be32_to_cpu(hv->type);
> >  		offset = be32_to_cpu(hv->offset);
> >  		version = be32_to_cpu(hv->version);
> > +		dbob_id = be32_to_cpu(hv->dbob_id);
> > +		prlog(PR_DEBUG, "entry %d: type=0x%x, version=0x%x, dbob_id=0x%x, offset=0x%x\n",
> > +				i, type, version, dbob_id, offset);
> >  
> 
> just trying to understand, if it is only for printing, why we need 
> "dbob_id" in stack, rest of them (like type, offset, ...) are used
> for dt node creations, cant we directly do "be32_to_cpu(hv->dbob_id)"
> at part of print?

Agree. Will print it without dbob_id variable.

Thanks for your review.
- Mahesh.