On Fri, Feb 23, 2024 at 5:03 PM Mickaël Salaün mic@digikod.net wrote:
On Fri, Feb 23, 2024 at 04:05:16PM -0500, Paul Moore wrote:
On Fri, Feb 23, 2024 at 3:04 PM Mickaël Salaün mic@digikod.net wrote:
On Fri, Feb 23, 2024 at 08:59:34PM +0100, Mickaël Salaün wrote:
On Fri, Feb 23, 2024 at 08:05:45PM +0100, Mickaël Salaün wrote:
selinux_lsm_getattr() may not initialize the value's pointer in some case. As for proc_pid_attr_read(), initialize this pointer to NULL in selinux_getselfattr() to avoid an UAF in the kfree() call.
Not UAF but NULL pointer dereference (both patches)...
Well, that may be the result (as observed with the kfree() call), but the cause is obviously an uninitialized pointer.
Adding the SELinux list to the CC line; SELinux folks the original post is here:
Thanks for finding this and testing the patch, based on our off-list discussion, do you mind if I add a Suggested-by? Looking at this a
Sure! I was in a hurry and didn't give it the attention it needed...
bit more I think we'll want to make a few changes to selinux_lsm_getattr() later, but this patch is a good one for stable as it not only fixes the bug, but it is a trivial one-liner with very low risk.
I do think we need to tweak the commit description a bit, what do you think of the following?
"selinux_getselfattr() doesn't properly initialize the string pointer it passes to selinux_lsm_getattr() which can cause a problem when an attribute hasn't been explicitly set; selinux_lsm_getattr() returns 0/success, but does not set or initialize the string label/attribute. Failure to properly initialize the string causes problems later in selinux_getselfattr() when the function attempts to kfree() the string."
Much better!
Great :) I just went ahead and merged this into the lsm/stable-6.8 branch to get this some testing in linux-next, although I'm going to be *shocked* if this commit causes a regression. I'll send this up to Linus early next week, and if John wants me to send the AppArmor patch I'll do that at the same time.