netdev - [bug report] mctp i2c: MCTP I2C binding driver

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>] [day] [month] [year] [list]

Message-ID: <20220224095154.GA32007@kili>
Date:   Thu, 24 Feb 2022 12:51:54 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     matt@...econstruct.com.au
Cc:     netdev@...r.kernel.org
Subject: [bug report] mctp i2c: MCTP I2C binding driver

Hello Matt Johnston,

The patch f5b8abf9fc3d: "mctp i2c: MCTP I2C binding driver" from Feb
18, 2022, leads to the following Smatch static checker warning:

	drivers/net/mctp/mctp-i2c.c:341 mctp_i2c_recv()
	error: dereferencing freed memory 'skb'

drivers/net/mctp/mctp-i2c.c
    271 static int mctp_i2c_recv(struct mctp_i2c_dev *midev)
    272 {
    273         struct net_device *ndev = midev->ndev;
    274         struct mctp_i2c_hdr *hdr;
    275         struct mctp_skb_cb *cb;
    276         struct sk_buff *skb;
    277         unsigned long flags;
    278         u8 pec, calc_pec;
    279         size_t recvlen;
    280         int status;
    281 
    282         /* + 1 for the PEC */
    283         if (midev->rx_pos < MCTP_I2C_MINLEN + 1) {
    284                 ndev->stats.rx_length_errors++;
    285                 return -EINVAL;
    286         }
    287         /* recvlen excludes PEC */
    288         recvlen = midev->rx_pos - 1;
    289 
    290         hdr = (void *)midev->rx_buffer;
    291         if (hdr->command != MCTP_I2C_COMMANDCODE) {
    292                 ndev->stats.rx_dropped++;
    293                 return -EINVAL;
    294         }
    295 
    296         if (hdr->byte_count + offsetof(struct mctp_i2c_hdr, source_slave) != recvlen) {
    297                 ndev->stats.rx_length_errors++;
    298                 return -EINVAL;
    299         }
    300 
    301         pec = midev->rx_buffer[midev->rx_pos - 1];
    302         calc_pec = i2c_smbus_pec(0, midev->rx_buffer, recvlen);
    303         if (pec != calc_pec) {
    304                 ndev->stats.rx_crc_errors++;
    305                 return -EINVAL;
    306         }
    307 
    308         skb = netdev_alloc_skb(ndev, recvlen);
    309         if (!skb) {
    310                 ndev->stats.rx_dropped++;
    311                 return -ENOMEM;
    312         }
    313 
    314         skb->protocol = htons(ETH_P_MCTP);
    315         skb_put_data(skb, midev->rx_buffer, recvlen);
    316         skb_reset_mac_header(skb);
    317         skb_pull(skb, sizeof(struct mctp_i2c_hdr));
    318         skb_reset_network_header(skb);
    319 
    320         cb = __mctp_cb(skb);
    321         cb->halen = 1;
    322         cb->haddr[0] = hdr->source_slave >> 1;
    323 
    324         /* We need to ensure that the netif is not used once netdev
    325          * unregister occurs
    326          */
    327         spin_lock_irqsave(&midev->lock, flags);
    328         if (midev->allow_rx) {
    329                 reinit_completion(&midev->rx_done);
    330                 spin_unlock_irqrestore(&midev->lock, flags);
    331 
    332                 status = netif_rx(skb);

The netif_rx() function frees the skb.

    333                 complete(&midev->rx_done);
    334         } else {
    335                 status = NET_RX_DROP;
    336                 spin_unlock_irqrestore(&midev->lock, flags);
    337         }
    338 
    339         if (status == NET_RX_SUCCESS) {
    340                 ndev->stats.rx_packets++;
--> 341                 ndev->stats.rx_bytes += skb->len;

Can we just do "ndev->stats.rx_bytes += recvlen;"?

    342         } else {
    343                 ndev->stats.rx_dropped++;
    344         }
    345         return 0;
    346 }
    347 
    348 enum mctp_i2c_flow_state {
    349         MCTP_I2C_TX_FLOW_INVALID,
    350         MCTP_I2C_TX_FLOW_NONE,
    351         MCTP_I2C_TX_FLOW_NEW,

regards,
dan carpenter