linux-kernel - Re: [PATCH] dlm: prevent NPD when writing a positive value to event

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: <CAK-6q+g3TYu5MKkwzcgDsBGjxZ3j5D-kxXdeFmQPv1dMxkSzRg@mail.gmail.com>
Date: Wed, 8 Jan 2025 09:27:31 -0500
From: Alexander Aring <aahringo@...hat.com>
To: Thadeu Lima de Souza Cascardo <cascardo@...lia.com>
Cc: gfs2@...ts.linux.dev, David Teigland <teigland@...hat.com>, linux-kernel@...r.kernel.org, 
	kernel-dev@...lia.com
Subject: Re: [PATCH] dlm: prevent NPD when writing a positive value to event_done

Hi,

On Tue, Jan 7, 2025 at 1:19 PM Thadeu Lima de Souza Cascardo
<cascardo@...lia.com> wrote:
>
> On Mon, Jan 06, 2025 at 11:14:35AM -0500, Alexander Aring wrote:
> > Hi,
> >
> > On Fri, Dec 20, 2024 at 9:28 AM Thadeu Lima de Souza Cascardo
> > <cascardo@...lia.com> wrote:
> > >
> > > do_uevent returns the value written to event_done. In case it is a positive
> > > value, new_lockspace would undo all the work, and lockspace would not be
> > > set. __dlm_new_lockspace, however, would treat that positive value as a
> > > success due to commit 8511a2728ab8 ("dlm: fix use count with multiple
> > > joins").
> > >
> > > Down the line, device_create_lockspace would pass that NULL lockspace to
> > > dlm_find_lockspace_local, leading to a NULL pointer dereference:
> > >
> >
> > This issue starts to be an issue since commit 4db41bf4f04f ("dlm:
> > remove ls_local_handle from struct dlm_ls"). Before
> > dlm_find_lockspace_local() was then returning NULL and
> > device_create_lockspace() returned -ENOENT as it wasn't found a NULL
> > lockspace handle, see [0].
> > After the patch it dlm_find_lockspace_local() will end in a
> > null-pointer dereference because we don't do this lookup of pointers
> > when we already should have this pointer already.
> > This behaviour is kind of weird (as it makes no sense to lookup NULL)
> > and yes, I didn't see that when I wrote commit 4db41bf4f04f ("dlm:
> > remove ls_local_handle from struct dlm_ls"). Just this commit signals
> > us we do something stupid with the error handling.
> >
> > May I ask you which user space software do you use? As for
> > dlm_controld I can't see that there is a positive number case for
> > telling the event_done attribute. [1]
>
> I was just playing with it manually, that is, without dlm_controld. I was
> writing down into configfs and sysfs manually while issuing ioctls and then
> tripped over this NULL pointer dereference.
>

ok, thanks... I am worried now you probably see more issues. :)

> I see how commit 4db41bf4f04f ("dlm: remove ls_local_handle from struct
> dlm_ls") leads to the NPD. In any case, I suppose the Fixes line could
> either be the one I pointed to (since that is what leads to such positive
> return values to undo lockspace creation and still be interpreted as
> success) or 4db41bf4f04f, specially if we want to restrict the backports to
> only the kernels with this latter commit.
>

yes, it was already broken before. I agree.

Acked-by: Alexander Aring <aahringo@...hat.com>

- Alex