linux-kernel - Re: [PATCH] misc: mei: client.c: fix some error code problem in mei_cl

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 for Android: free password hash cracker in your pocket

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

Message-ID: <92972476-0b1f-4d0a-9951-af3fc8bc6e65@suswa.mountain>
Date:   Mon, 20 Nov 2023 10:03:41 -0500
From:   Dan Carpenter <dan.carpenter@...aro.org>
To:     Su Hui <suhui@...china.com>
Cc:     tomas.winkler@...el.com, arnd@...db.de, gregkh@...uxfoundation.org,
        nathan@...nel.org, ndesaulniers@...gle.com, trix@...hat.com,
        alexander.usyskin@...el.com, linux-kernel@...r.kernel.org,
        llvm@...ts.linux.dev, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] misc: mei: client.c: fix some error code problem in
 mei_cl_write

On Mon, Nov 20, 2023 at 04:53:45PM +0800, Su Hui wrote:
> Clang static analyzer complains that value stored to 'rets' is never
> read. Remove some useless code, and let 'buf_len = -EOVERFLOW' to make
> sure we can return '-EOVERFLOW'.
> 
> mei_msg_hdr_init() return negative error code, rets should be
> 'PTR_ERR(mei_hdr)' rather than '-PTR_ERR(mei_hdr)'.
> 
> Fixes: 0cd7c01a60f8 ("mei: add support for mei extended header.")
> Fixes: 8c8d964ce90f ("mei: move hbuf_depth from the mei device to the hw modules")
> Signed-off-by: Su Hui <suhui@...china.com>
> ---
>  drivers/misc/mei/client.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
> index 9c8fc87938a7..00dac0a47da0 100644
> --- a/drivers/misc/mei/client.c
> +++ b/drivers/misc/mei/client.c
> @@ -2011,7 +2011,7 @@ ssize_t mei_cl_write(struct mei_cl *cl, struct mei_cl_cb *cb, unsigned long time
>  
>  	mei_hdr = mei_msg_hdr_init(cb);
>  	if (IS_ERR(mei_hdr)) {
> -		rets = -PTR_ERR(mei_hdr);
> +		rets = PTR_ERR(mei_hdr);

KTODO: write a static checker rule which complains -PTR_ERR()

This might be complicated because there are parts of networking where
we store error codes as positive values.  But there is enough context in
this function to create some sort of warning about this code.  "Mixing
positive and negative error codes" perhaps?

$ git grep -n '\-PTR_ERR'
block/partitions/core.c:574:                   disk->disk_name, p, -PTR_ERR(part));
drivers/infiniband/ulp/ipoib/ipoib_multicast.c:291:                        -PTR_ERR(ah));
drivers/misc/mei/client.c:2014:         rets = -PTR_ERR(mei_hdr);
drivers/net/ethernet/intel/igb/igb_main.c:8963:                 unsigned int xdp_res = -PTR_ERR(skb);
drivers/net/ethernet/intel/igc/igc_main.c:2639:                 unsigned int xdp_res = -PTR_ERR(skb);
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:2348:                     unsigned int xdp_res = -PTR_ERR(skb);
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:5431:                         unsigned int xdp_res = -PTR_ERR(skb);
drivers/phy/sunplus/phy-sunplus-usb2.c:278:             ret = -PTR_ERR(phy);
drivers/scsi/libfc/fc_elsct.c:86:               switch (-PTR_ERR(fp)) {
drivers/scsi/libfc/fc_lport.c:1081:                  IS_ERR(fp) ? -PTR_ERR(fp) : 0, fc_lport_state(lport),
fs/ext4/indirect.c:1042:                                ext4_error_inode_block(inode, nr, -PTR_ERR(bh),
fs/jffs2/background.c:48:                       -PTR_ERR(tsk));
fs/ntfs/dir.c:92:                               -PTR_ERR(m));
fs/ntfs/dir.c:312:                              -PTR_ERR(page));
fs/ntfs/dir.c:643:                              -PTR_ERR(m));
fs/ntfs/dir.c:790:                              -PTR_ERR(page));
fs/ntfs/index.c:141:                            -PTR_ERR(m));
fs/ntfs/index.c:268:                            -PTR_ERR(page));
fs/ntfs/mft.c:162:      ntfs_error(ni->vol->sb, "Failed with error code %lu.", -PTR_ERR(m));
fs/ntfs/mft.c:289:                              "mft record, error code %ld.", -PTR_ERR(m));
net/ipv6/af_inet6.c:852:                        WRITE_ONCE(sk->sk_err_soft, -PTR_ERR(dst));
net/ipv6/inet6_connection_sock.c:123:           WRITE_ONCE(sk->sk_err_soft, -PTR_ERR(dst));
tools/lib/bpf/libbpf.c:10044:           errno = -PTR_ERR(ptr);
tools/lib/bpf/libbpf_internal.h:520:            errno = -PTR_ERR(ret);
tools/testing/selftests/bpf/prog_tests/btf_dump.c:59:           err = -PTR_ERR(btf);
tools/testing/selftests/bpf/prog_tests/sk_lookup.c:483:         errno = -PTR_ERR(link);

Quite a few of those were in printks and it might be an opportunity to
use %pe to print the ENOMEM etc strings instead of the number.

regards,
dan carpenter