| 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
| ||
|
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