linux-kernel - Re: [PATCH] s390/cio: Fix a memleak in css_alloc

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-next>] [day] [month] [year] [list]

Message-ID: <20230922141700.10895474.pasic@linux.ibm.com>
Date:   Fri, 22 Sep 2023 14:17:00 +0200
From:   Halil Pasic <pasic@...ux.ibm.com>
To:     Dinghao Liu <dinghao.liu@....edu.cn>
Cc:     Vineeth Vijayan <vneethv@...ux.ibm.com>,
        Peter Oberparleiter <oberpar@...ux.ibm.com>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Alexander Gordeev <agordeev@...ux.ibm.com>,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Sven Schnelle <svens@...ux.ibm.com>,
        Julian Wiedmann <jwi@...ux.ibm.com>,
        linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
        Halil Pasic <pasic@...ux.ibm.com>
Subject: Re: [PATCH] s390/cio: Fix a memleak in css_alloc_subchannel

On Thu, 21 Sep 2023 15:14:12 +0800
Dinghao Liu <dinghao.liu@....edu.cn> wrote:

> When dma_set_coherent_mask() fails, sch->lock has not been
> freed, which is allocated in css_sch_create_locks(), leading
> to a memleak.
> 
> Fixes: 4520a91a976e ("s390/cio: use dma helpers for setting masks")
> Signed-off-by: Dinghao Liu <dinghao.liu@....edu.cn>

Reviewed-by: Halil Pasic <pasic@...ux.ibm.com>

@Vineeth: Do you know why is the spinlock "*sch->lock" allocated
dynamically and referenced via a pointer instead of making the
spinlock simply a member of struct subchannel and getting rid
of the extra allocation?

I did some archaeology together with Peter. The
lock used to be a member but then commit 2ec2298412e1 ("[S390]
subchannel lock conversion.") switched to (mostly) allocating
the lock separately. Mostly because of this hunk:

@@ -520,9 +530,15 @@ cio_validate_subchannel (struct subchannel *sch, struct subchannel_id schid)
        /* Nuke all fields. */
        memset(sch, 0, sizeof(struct subchannel));

-       spin_lock_init(&sch->lock);
+       sch->schid = schid;
+       if (cio_is_console(schid)) {
+               sch->lock = cio_get_console_lock();
+       } else {
+               err = cio_create_sch_lock(sch);
+               if (err)
+                       goto out;
+       }

I did not spend a huge amount of time looking at this but this
is the only reason I found for sch->lock being made a pointer. There may
be others, I'm just saying that is all I've found.

Since 863fc8492734 ("s390/cio: get rid of static console subchannel")
that reason with the console_lock is no more. And that brings me back to
the question: "Why?"

Regards,
Halil

[..]