linux-kernel - Re: [PATCH] lib80211: Use ERR

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: <20240907200846.GJ1049718@ZenIV>
Date: Sat, 7 Sep 2024 21:08:46 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Rohit Chavan <roheetchavan@...il.com>
Cc: Johannes Berg <johannes@...solutions.net>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] lib80211: Use ERR_CAST() to return

On Fri, Sep 06, 2024 at 05:14:55PM +0530, Rohit Chavan wrote:
> Using ERR_CAST() is more reasonable and safer, When it is necessary
> to convert the type of an error pointer and return it.

Why is it safer?

Please, explain the reasoning that had lead you to this conclusion.
This is a serious request, BTW.

>From my reading of that code (and its well-hidden callers - AFAICS, those
are
drivers/net/wireless/intel/ipw2x00/libipw_wx.c:382:                     new_crypt->priv = new_crypt->ops->init(key);
drivers/net/wireless/intel/ipw2x00/libipw_wx.c:611:                     new_crypt->priv = new_crypt->ops->init(idx);
drivers/staging/rtl8192e/rtllib_wx.c:347:                       new_crypt->priv = new_crypt->ops->init(key);
drivers/staging/rtl8192e/rtllib_wx.c:570:                       new_crypt->priv = new_crypt->ops->init(idx);
) what we have is unfortunate calling conventions for that ->init() method.

It reports failures by returning NULL; that's not a good idea for
something that is supposed to allocate a crypto-algorithm-specific
object, since anything that does *NOT* need any allocations at all
can't just return NULL.

This "(void *)1" is basically "let me invent some non-NULL pointer,
I won't be using it at all".  Yes, it's a kludge; there are more idiomatic
ways, but that really ought to be discussed with maintainers, especially
since there are only two places using that sucker in the first place,
one of them being in staging...

In any case, that (void *)1 is _not_ something you want to express as
ERR_CAST() (or ERR_PTR(), for that matter); that would only make the damn
thing harder for readers.

Speaking of making things harder for readers, near the 4th of these callers
there's something weird:
        ops = lib80211_get_crypto_ops(alg);
        if (!ops) {
                char tempbuf[100];

                memset(tempbuf, 0x00, 100);
                sprintf(tempbuf, "%s", module);
                request_module("%s", tempbuf);
                ops = lib80211_get_crypto_ops(alg);
        }

What the hell is going on there?  module is one of the "rtllib_crypt_wep",
"rtllib_crypt_tkip" and "rtllib_crypt_ccmp"...  Looks like a very odd
obfuscation; other callers also come with calls of request_module(),
but those don't do that insane dance...