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] [day] [month] [year] [list]
Date: Tue, 14 Nov 2023 17:42:44 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Suman Ghosh <sumang@...vell.com>, sgoutham@...vell.com,
 gakula@...vell.com, sbhatta@...vell.com, hkelam@...vell.com,
 lcherian@...vell.com, jerinj@...vell.com, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org, horms@...nel.org
Cc: Ratheesh Kannoth <rkannoth@...vell.com>
Subject: Re: [net PATCH] octeontx2: Fix klockwork and coverity issues

Le 01/11/2023 à 08:49, Suman Ghosh a écrit :
> Fix all klockwork and coverity issues reported on AF and PF/VF driver.
> 
> Signed-off-by: Suman Ghosh <sumang@...vell.com>
> Signed-off-by: Ratheesh Kannoth <rkannoth@...vell.com>
> ---
>   .../net/ethernet/marvell/octeontx2/af/cgx.c   | 14 ++++-
>   .../marvell/octeontx2/af/mcs_rvu_if.c         |  8 ++-
>   .../net/ethernet/marvell/octeontx2/af/ptp.c   | 11 +++-
>   .../ethernet/marvell/octeontx2/af/rvu_cpt.c   |  2 +-
>   .../marvell/octeontx2/af/rvu_debugfs.c        |  8 ++-
>   .../ethernet/marvell/octeontx2/af/rvu_nix.c   |  2 +-
>   .../ethernet/marvell/octeontx2/af/rvu_npc.c   |  2 +-
>   .../marvell/octeontx2/nic/otx2_common.c       |  8 +--
>   .../ethernet/marvell/octeontx2/nic/otx2_pf.c  |  3 +
>   .../ethernet/marvell/octeontx2/nic/otx2_reg.h | 55 ++++++++++---------
>   .../marvell/octeontx2/nic/otx2_txrx.c         |  2 +-
>   .../net/ethernet/marvell/octeontx2/nic/qos.c  |  7 ++-
>   12 files changed, 77 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cgx.c b/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
> index 6c70c8498690..5a672888577e 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cgx.c
> @@ -457,12 +457,19 @@ int cgx_lmac_addr_max_entries_get(u8 cgx_id, u8 lmac_id)
>   u64 cgx_lmac_addr_get(u8 cgx_id, u8 lmac_id)
>   {
>   	struct cgx *cgx_dev = cgx_get_pdata(cgx_id);
> -	struct lmac *lmac = lmac_pdata(lmac_id, cgx_dev);
>   	struct mac_ops *mac_ops;
> +	struct lmac *lmac;
>   	int index;
>   	u64 cfg;
>   	int id;
>   
> +	if (!cgx_dev)
> +		return 0;
> +
> +	lmac = lmac_pdata(lmac_id, cgx_dev);
> +	if (!lmac)
> +		return 0;
> +
>   	mac_ops = cgx_dev->mac_ops;
>   
>   	id = get_sequence_id_of_lmac(cgx_dev, lmac_id);
> @@ -955,6 +962,9 @@ int cgx_lmac_pfc_config(void *cgxd, int lmac_id, u8 tx_pause,
>   
>   	/* Write source MAC address which will be filled into PFC packet */
>   	cfg = cgx_lmac_addr_get(cgx->cgx_id, lmac_id);
> +	if (!cfg)
> +		return -ENODEV;
> +
>   	cgx_write(cgx, lmac_id, CGXX_SMUX_SMAC, cfg);
>   
>   	return 0;
> @@ -1617,7 +1627,7 @@ unsigned long cgx_get_lmac_bmap(void *cgxd)
>   static int cgx_lmac_init(struct cgx *cgx)
>   {
>   	struct lmac *lmac;
> -	u64 lmac_list;
> +	u64 lmac_list = 0;

This is not needed.
The static checker is not good enough to see it, that's all :).

>   	int i, err;
>   
>   	/* lmac_list specifies which lmacs are enabled
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c b/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c
> index dfd23580e3b8..1b0b022f5493 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/mcs_rvu_if.c
> @@ -625,8 +625,8 @@ int rvu_mbox_handler_mcs_free_resources(struct rvu *rvu,
>   {
>   	u16 pcifunc = req->hdr.pcifunc;
>   	struct mcs_rsrc_map *map;
> +	int rc = -EINVAL;
>   	struct mcs *mcs;
> -	int rc = 0;

This should be argumented.

This changes the behavior of the code. Why should we return -EINVAL if 
nothing in the switch below matches?

>   
>   	if (req->mcs_id >= rvu->mcs_blk_cnt)
>   		return MCS_AF_ERR_INVALID_MCSID;
> @@ -675,8 +675,8 @@ int rvu_mbox_handler_mcs_alloc_resources(struct rvu *rvu,
>   {
>   	u16 pcifunc = req->hdr.pcifunc;
>   	struct mcs_rsrc_map *map;
> +	int rsrc_id = -EINVAL, i;
>   	struct mcs *mcs;
> -	int rsrc_id, i;

Same

>   
>   	if (req->mcs_id >= rvu->mcs_blk_cnt)
>   		return MCS_AF_ERR_INVALID_MCSID;
> @@ -737,6 +737,8 @@ int rvu_mbox_handler_mcs_alloc_resources(struct rvu *rvu,
>   			rsp->rsrc_cnt++;
>   		}
>   		break;
> +	default:
> +		goto exit;
>   	}
>   
>   	rsp->rsrc_type = req->rsrc_type;
> @@ -849,7 +851,7 @@ int rvu_mbox_handler_mcs_ctrl_pkt_rule_write(struct rvu *rvu,
>   static void rvu_mcs_set_lmac_bmap(struct rvu *rvu)
>   {
>   	struct mcs *mcs = mcs_get_pdata(0);
> -	unsigned long lmac_bmap;
> +	unsigned long lmac_bmap = 0;
>   	int cgx, lmac, port;
>   
>   	for (port = 0; port < mcs->hw->lmac_cnt; port++) {
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/ptp.c b/drivers/net/ethernet/marvell/octeontx2/af/ptp.c
> index bcc96eed2481..a199b1123ba7 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/ptp.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/ptp.c
> @@ -518,6 +518,7 @@ static int ptp_probe(struct pci_dev *pdev,
>   		     const struct pci_device_id *ent)
>   {
>   	struct ptp *ptp;
> +	void __iomem * const *base;
>   	int err;
>   
>   	ptp = kzalloc(sizeof(*ptp), GFP_KERNEL);
> @@ -536,7 +537,15 @@ static int ptp_probe(struct pci_dev *pdev,
>   	if (err)
>   		goto error_free;
>   
> -	ptp->reg_base = pcim_iomap_table(pdev)[PCI_PTP_BAR_NO];
> +	base = pcim_iomap_table(pdev);
> +	if (!base)
> +		goto error_free;
> +
> +	ptp->reg_base = base[PCI_PTP_BAR_NO];
> +	if (!ptp->reg_base) {
> +		err = -ENODEV;
> +		goto error_free;
> +	}
>   
>   	pci_set_drvdata(pdev, ptp);
>   	if (!first_ptp_block)
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c
> index f047185f38e0..a1a919fcda47 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c
> @@ -43,7 +43,7 @@ static irqreturn_t cpt_af_flt_intr_handler(int vec, void *ptr)
>   	struct rvu *rvu = block->rvu;
>   	int blkaddr = block->addr;
>   	u64 reg, val;
> -	int i, eng;
> +	int i, eng = 0;

Why is this the correct value if nothing else matches in the switch below?

>   	u8 grp;
>   
>   	reg = rvu_read64(rvu, blkaddr, CPT_AF_FLTX_INT(vec));
> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
> index bd817ee88735..307942ff1b10 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_debugfs.c
> @@ -519,12 +519,16 @@ RVU_DEBUG_SEQ_FOPS(mcs_rx_secy_stats, mcs_rx_secy_stats_display, NULL);
>   static void rvu_dbg_mcs_init(struct rvu *rvu)
>   {
>   	struct mcs *mcs;
> -	char dname[10];
> +	char *dname = NULL;

=NULL is not needed, but see below.

>   	int i;
>   
>   	if (!rvu->mcs_blk_cnt)
>   		return;
>   
> +	dname = kmalloc_array(rvu->mcs_blk_cnt, sizeof(char), GFP_KERNEL);

Hi,

I think that kmalloc() would be enough here.

More importantly, I think it is wrong.
This dbname buffer is used to store "mcs%d". %d can be up to 
rvu->mcs_blk_cnt, but it does not require that amount of memory.

(if mcs_blk_cnt = 1000, we need 3 + 4 + 1 = 8 byte ("mcs" + "1000" + 
\0), not 1000 bytes.

If mcs_blk_cnt is small, we under-allocate, if it is high we allocate 
way too much memory.

Maybe kasprintf()/kfree() would be a cleaner option.


> +	if (!dname)
> +		return;
> +
>   	rvu->rvu_dbg.mcs_root = debugfs_create_dir("mcs", rvu->rvu_dbg.root);
>   
>   	for (i = 0; i < rvu->mcs_blk_cnt; i++) {
> @@ -568,6 +572,8 @@ static void rvu_dbg_mcs_init(struct rvu *rvu)
>   		debugfs_create_file("port", 0600, rvu->rvu_dbg.mcs_tx, mcs,
>   				    &rvu_dbg_mcs_tx_port_stats_fops);
>   	}
> +
> +	kfree(dname);
>   }
>   

...

(I have not looked the code below that)


Powered by blists - more mailing lists