linux-kernel - RE: [PATCH] cleanup: Add usage and style documentation

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: <65fe1f9aadf51_2690d2948f@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Date: Fri, 22 Mar 2024 17:17:30 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: "Tian, Kevin" <kevin.tian@...el.com>, "Williams, Dan J"
	<dan.j.williams@...el.com>, "peterz@...radead.org" <peterz@...radead.org>,
	"torvalds@...ux-foundation.org" <torvalds@...ux-foundation.org>
CC: Bjorn Helgaas <bhelgaas@...gle.com>, "Weiny, Ira" <ira.weiny@...el.com>,
	Jonathan Cameron <jonathan.cameron@...wei.com>, "Brandeburg, Jesse"
	<jesse.brandeburg@...el.com>, Ilpo Järvinen
	<ilpo.jarvinen@...ux.intel.com>, "Wunner, Lukas" <lukas.wunner@...el.com>,
	Jonathan Corbet <corbet@....net>, "linux-pci@...r.kernel.org"
	<linux-pci@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "gregkh@...uxfoundation.org"
	<gregkh@...uxfoundation.org>, "linux-doc@...r.kernel.org"
	<linux-doc@...r.kernel.org>
Subject: RE: [PATCH] cleanup: Add usage and style documentation

Tian, Kevin wrote:
> > From: Dan Williams <dan.j.williams@...el.com>
> > Sent: Thursday, March 21, 2024 6:05 AM
> > + *
> > + * Note that unwind order is dictated by declaration order. That
> > + * contraindicates a pattern like the following:
> > + *
> > + * .. code-block:: c
> > + *
> > + *	int num, ret = 0;
> > + *	struct pci_dev *bridge = ctrl->pcie->port;
> > + *	struct pci_bus *parent = bridge->subordinate;
> > + *	struct pci_dev *dev __free(pci_dev_put) = NULL;
> > + *
> > + *	pci_lock_rescan_remove();
> > + *
> > + *	dev = pci_get_slot(parent, PCI_DEVFN(0, 0));
> > + *
> > + * In this case @dev is declared in x-mas tree style in a preamble
> > + * declaration block. That is problematic because it destroys the
> > + * compiler's ability to infer proper unwind order. If other cleanup
> > + * helpers appeared in such a function that depended on @dev being live
> > + * to complete their unwind then using the "struct obj_type *obj
> > + * __free(...) = NULL" style is an anti-pattern that potentially causes
> > + * a use-after-free bug. Instead, the expectation is this conversion:
> > + *
> 
> an example of dependent cleanup helpers might be helpful to
> better understand this expectation?

The simplest example I can think of to show the danger of the
"__free(...) = NULL" causing cleanup inter-dependency problems is the
following:

---
LIST_HEAD(list);
DEFINE_MUTEX(lock);

struct object {
        struct list_head node;
};

static struct object *alloc_add(void)
{
        struct object *obj;

        lockdep_assert_held(&lock);
        obj = kfree(sizeof(*obj), GFP_KERNEL);
        if (obj) {
                LIST_HEAD_INIT(&obj->node);
		list_add(obj->node, &list):
	}
        return obj;
}

static void remove_free(struct object *obj)
{
        lockdep_assert_held(&lock);
        list_del(&obj->node);
        kfree(obj);
}

DEFINE_FREE(remove_free, struct object *, if (_T) remove_free(_T))
static int init(void)
{
        struct object *obj __free(remove_free) = NULL;
        int err;

        guard(mutex)(lock);
        obj = alloc_add();

        if (!obj)
                return -ENOMEM;

        err = other_init(obj);
        if (err)
                return err; // remove_free() called without the lock!!

        no_free_ptr(obj);
        return 0;
}
---

The fix for this bug is to replace the "__free(...) = NULL" pattern and
move the assignment to the declaration.

        guard(mutex)(lock);
        struct object *obj __free(remove_free) = alloc_add();

..so the compiler can observe LIFO order on the unwind. Yes, no one
should write code like this, all of the init should happen before
assigning to a list, but hopefully it illustrates the point.