linux-kernel - RE: [EXTERNAL] Re: [PATCH net,1/6] octeontx2-af: replace cpt slot with lf id on reg write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: 
 <DS0PR18MB5368AB102893ADAA1C5FA0FDA0DB2@DS0PR18MB5368.namprd18.prod.outlook.com>
Date: Tue, 9 Jul 2024 10:56:13 +0000
From: Srujana Challa <schalla@...vell.com>
To: Michal Kubiak <michal.kubiak@...el.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "davem@...emloft.net"
	<davem@...emloft.net>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        Sunil Kovvuri
 Goutham <sgoutham@...vell.com>,
        Linu Cherian <lcherian@...vell.com>,
        Geethasowjanya Akula <gakula@...vell.com>,
        Jerin Jacob <jerinj@...vell.com>,
        Hariprasad Kelam <hkelam@...vell.com>,
        Subbaraya Sundeep Bhatta
	<sbhatta@...vell.com>,
        Nithin Kumar Dabilpuram <ndabilpuram@...vell.com>
Subject: RE: [EXTERNAL] Re: [PATCH net,1/6] octeontx2-af: replace cpt slot
 with lf id on reg write

> > From: Nithin Dabilpuram <ndabilpuram@...vell.com>
> >
> > Replace cpt slot id with lf id on reg read/write as CPTPF/VF driver
> > would send slot number instead of lf id in the reg offset.
> >
> > Fixes: ae454086e3c2 ("octeontx2-af: add mailbox interface for CPT")
> > Signed-off-by: Nithin Dabilpuram <ndabilpuram@...vell.com>
> > ---
> >  drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c
> > b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c
> > index f047185f38e0..98440a0241a2 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c
> > @@ -663,6 +663,8 @@ static bool is_valid_offset(struct rvu *rvu, struct
> cpt_rd_wr_reg_msg *req)
> >  		if (lf < 0)
> >  			return false;
> >
> > +		req->reg_offset &= 0xFF000;
> > +		req->reg_offset += lf << 3;
> 
> I think it's not great to modify an input parameter from the function named
> like "is_valid_offset()". From the function like that I would rather expect doing
> just a simple check if the parameter is correct.
> It seems calling that function from a different context can be risky now.
I’ll make the necessary changes to the code to ensure that we avoid modifying
the input parameter within the is_valid_offset() function in next version.

> 
> >  		return true;
> >  	} else if (!(req->hdr.pcifunc & RVU_PFVF_FUNC_MASK)) {
> >  		/* Registers that can be accessed from PF */ @@ -707,12
> +709,13 @@
> > int rvu_mbox_handler_cpt_rd_wr_register(struct rvu *rvu,
> >  	    !is_cpt_vf(rvu, req->hdr.pcifunc))
> >  		return CPT_AF_ERR_ACCESS_DENIED;
> >
> > +	if (!is_valid_offset(rvu, req))
> > +		return CPT_AF_ERR_ACCESS_DENIED;
> > +
> >  	rsp->reg_offset = req->reg_offset;
> >  	rsp->ret_val = req->ret_val;
> >  	rsp->is_write = req->is_write;
> >
> > -	if (!is_valid_offset(rvu, req))
> > -		return CPT_AF_ERR_ACCESS_DENIED;
> 
> Is moving that call also a necessary part of the fix? Or is it just an extra
> improvement?
> Maybe it's worth mentioning in the commit message?
is_valid_offset() call is moved to ensure that rsp->reg_offset is correctly updated
with the modified req->reg_offset.

Thanks for the feedback.
> 
> >
> >  	if (req->is_write)
> >  		rvu_write64(rvu, blkaddr, req->reg_offset, req->val);
> > --
> > 2.25.1
> >
> >
> 
> 
> Thanks,
> Michal