netdev - Re: [PATCH 1/3] s390/vmlogrdr: Remove function pointer cast

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: <20240418203232.GA2962980@dev-arch.thelio-3990X>
Date: Thu, 18 Apr 2024 13:32:32 -0700
From: Nathan Chancellor <nathan@...nel.org>
To: Heiko Carstens <hca@...ux.ibm.com>
Cc: akpm@...ux-foundation.org, arnd@...db.de, gor@...ux.ibm.com,
	agordeev@...ux.ibm.com, borntraeger@...ux.ibm.com,
	svens@...ux.ibm.com, wintera@...ux.ibm.com, twinkler@...ux.ibm.com,
	linux-s390@...r.kernel.org, netdev@...r.kernel.org,
	llvm@...ts.linux.dev, patches@...ts.linux.dev
Subject: Re: [PATCH 1/3] s390/vmlogrdr: Remove function pointer cast

On Thu, Apr 18, 2024 at 09:21:00PM +0200, Heiko Carstens wrote:
> Hi Nathan,
> 
> > > > > > > -		/*
> > > > > > > -		 * The release function could be called after the
> > > > > > > -		 * module has been unloaded. It's _only_ task is to
> > > > > > > -		 * free the struct. Therefore, we specify kfree()
> > > > > > > -		 * directly here. (Probably a little bit obfuscating
> > > > > > > -		 * but legitime ...).
> > > > > > > -		 */
> > > 
> > > That doesn't answer my question what prevents the release function
> > > from being called after the module has been unloaded.
> > > 
> > > At least back then when the code was added it was a real bug.
> > 
> > I do not know the answer to that question (and I suspect there is
> > nothing preventing ->release() from being called after module unload),
> > so I'll just bring back the comment (although I'll need to adjust it
> > since kfree() is not being used there directly anymore). Andrew, would
> > you prefer a diff from what's in -mm or a v2?
> 
> I guess there is some confusion here :) My request was not to keep the

Heh, yes, my apologies for being rather dense, I was not interpreting
the comment or the thread you linked properly... :(

> comment. I'm much rather afraid that the comment is still valid; and if
> that is the case then your patch series adds three bugs, exactly what is
> described in the comment.
> 
> Right now the release function is kfree which is always within the kernel
> image, and therefore always a valid branch target. If however the code is
> changed to what you propose, then the release function would be inside of
> the module, which potentially does not exist anymore when the release
> function is called, since the module was unloaded.
> So the branch target would be invalid.

That is super subtle :/ I can understand what the comment is warning
about with that extra context. I see Arnd's suggestion which may fix
this problem and get rid of the warning but if there are other ideas, I
am all ears. I guess we could just disable -Wcast-function-type-strict
for this code since s390 does not support kCFI right now but since it
could, it seems better to resolve it properly.

Thanks a lot for the quick review and catching my mistake, cheers!
Nathan