Re: [PATCH 10/12] s390/ap: use generic driver_override infrastructure

Harald Freudenberger
2026-03-24T12:42:20Z

On 2026-03-24 01:59, Danilo Krummrich wrote:
> When the AP masks are updated via apmask_store() or aqmask_store(),
> ap_bus_revise_bindings() is called after ap_attr_mutex has been
> released.
>
> This calls __ap_revise_reserved(), which accesses the driver_override
> field without holding any lock, racing against a concurrent
> driver_override_store() that may free the old string, resulting in a
> potential UAF.
>
> Fix this by using the driver-core driver_override infrastructure, which
> protects all accesses with an internal spinlock.
>
> Note that unlike most other buses, the AP bus does not check
> driver_override in its match() callback; the override is checked in
> ap_device_probe() and __ap_revise_reserved() instead.
>
> Also note that we do not enable the driver_override feature of struct
> bus_type, as AP - in contrast to most other buses - passes "" to
> sysfs_emit() when the driver_override pointer is NULL. Thus, printing
> "\n" instead of "(null)\n".
>
> Additionally, AP has a custom counter that is modified in the
> corresponding custom driver_override_store().
>
> Fixes: d38a87d7c064 ("s390/ap: Support driver_override for AP queue
> devices")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
> drivers/s390/crypto/ap_bus.c | 34 +++++++++++++++++-----------------
> drivers/s390/crypto/ap_bus.h | 1 -
> drivers/s390/crypto/ap_queue.c | 24 ++++++------------------
> 3 files changed, 23 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/s390/crypto/ap_bus.c
> b/drivers/s390/crypto/ap_bus.c
> index d652df96a507..f24e27add721 100644
> --- a/drivers/s390/crypto/ap_bus.c
> +++ b/drivers/s390/crypto/ap_bus.c
> @@ -859,25 +859,24 @@ static int
> __ap_queue_devices_with_id_unregister(struct device *dev, void *data)
>
> static int __ap_revise_reserved(struct device *dev, void *dummy)
> {
> - int rc, card, queue, devres, drvres;
> + int rc, card, queue, devres, drvres, ovrd;
>
> if (is_queue_dev(dev)) {
> struct ap_driver *ap_drv = to_ap_drv(dev->driver);
> struct ap_queue *aq = to_ap_queue(dev);
> - struct ap_device *ap_dev = &aq->ap_dev;
>
> card = AP_QID_CARD(aq->qid);
> queue = AP_QID_QUEUE(aq->qid);
>
> - if (ap_dev->driver_override) {
> - if (strcmp(ap_dev->driver_override,
> - ap_drv->driver.name)) {
> - pr_debug("reprobing queue=%02x.%04x\n", card, queue);
> - rc = device_reprobe(dev);
> - if (rc) {
> - AP_DBF_WARN("%s reprobing queue=%02x.%04x failed\n",
> - __func__, card, queue);
> - }
> + ovrd = device_match_driver_override(dev, &ap_drv->driver);
> + if (ovrd > 0) {
> + /* override set and matches, nothing to do */
> + } else if (ovrd == 0) {
> + pr_debug("reprobing queue=%02x.%04x\n", card, queue);
> + rc = device_reprobe(dev);
> + if (rc) {
> + AP_DBF_WARN("%s reprobing queue=%02x.%04x failed\n",
> + __func__, card, queue);
> }
> } else {
> mutex_lock(&ap_attr_mutex);
> @@ -928,7 +927,7 @@ int ap_owned_by_def_drv(int card, int queue)
> if (aq) {
> const struct device_driver *drv = aq->ap_dev.device.driver;
> const struct ap_driver *ap_drv = to_ap_drv(drv);
> - bool override = !!aq->ap_dev.driver_override;
> + bool override = device_has_driver_override(&aq->ap_dev.device);
>
> if (override && drv && ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
> rc = 1;
> @@ -977,7 +976,7 @@ static int ap_device_probe(struct device *dev)
> {
> struct ap_device *ap_dev = to_ap_dev(dev);
> struct ap_driver *ap_drv = to_ap_drv(dev->driver);
> - int card, queue, devres, drvres, rc = -ENODEV;
> + int card, queue, devres, drvres, rc = -ENODEV, ovrd;
>
> if (!get_device(dev))
> return rc;
> @@ -991,10 +990,11 @@ static int ap_device_probe(struct device *dev)
> */
> card = AP_QID_CARD(to_ap_queue(dev)->qid);
> queue = AP_QID_QUEUE(to_ap_queue(dev)->qid);
> - if (ap_dev->driver_override) {
> - if (strcmp(ap_dev->driver_override,
> - ap_drv->driver.name))
> - goto out;
> + ovrd = device_match_driver_override(dev, &ap_drv->driver);
> + if (ovrd > 0) {
> + /* override set and matches, nothing to do */
> + } else if (ovrd == 0) {
> + goto out;
> } else {
> mutex_lock(&ap_attr_mutex);
> devres = test_bit_inv(card, ap_perms.apm) &&
> diff --git a/drivers/s390/crypto/ap_bus.h
> b/drivers/s390/crypto/ap_bus.h
> index 51e08f27bd75..04ea256ecf91 100644
> --- a/drivers/s390/crypto/ap_bus.h
> +++ b/drivers/s390/crypto/ap_bus.h
> @@ -166,7 +166,6 @@ void ap_driver_unregister(struct ap_driver *);
> struct ap_device {
> struct device device;
> int device_type; /* AP device type. */
> - const char *driver_override;
> };
>
> #define to_ap_dev(x) container_of((x), struct ap_device, device)
> diff --git a/drivers/s390/crypto/ap_queue.c
> b/drivers/s390/crypto/ap_queue.c
> index 3fe2e41c5c6b..ca9819e6f7e7 100644
> --- a/drivers/s390/crypto/ap_queue.c
> +++ b/drivers/s390/crypto/ap_queue.c
> @@ -734,26 +734,14 @@ static ssize_t driver_override_show(struct device
> *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - struct ap_queue *aq = to_ap_queue(dev);
> - struct ap_device *ap_dev = &aq->ap_dev;
> - int rc;
> -
> - device_lock(dev);
> - if (ap_dev->driver_override)
> - rc = sysfs_emit(buf, "%s\n", ap_dev->driver_override);
> - else
> - rc = sysfs_emit(buf, "\n");
> - device_unlock(dev);
> -
> - return rc;
> + guard(spinlock)(&dev->driver_override.lock);
> + return sysfs_emit(buf, "%s\n", dev->driver_override.name ?: "");
> }
>
> static ssize_t driver_override_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - struct ap_queue *aq = to_ap_queue(dev);
> - struct ap_device *ap_dev = &aq->ap_dev;
> int rc = -EINVAL;
> bool old_value;
>
> @@ -764,13 +752,13 @@ static ssize_t driver_override_store(struct
> device *dev,
> if (ap_apmask_aqmask_in_use)
> goto out;
>
> - old_value = ap_dev->driver_override ? true : false;
> - rc = driver_set_override(dev, &ap_dev->driver_override, buf, count);
> + old_value = device_has_driver_override(dev);
> + rc = __device_set_driver_override(dev, buf, count);
> if (rc)
> goto out;
> - if (old_value && !ap_dev->driver_override)
> + if (old_value && !device_has_driver_override(dev))
> --ap_driver_override_ctr;
> - else if (!old_value && ap_dev->driver_override)
> + else if (!old_value && device_has_driver_override(dev))
> ++ap_driver_override_ctr;
>
> rc = count;

Thanks Danilo
Reviewed-by: Harald Freudenberger <freude@linux.ibm.com>