greybus-dev

greybus-dev@lists.linaro.org

33 participants
904 discussions

[PATCH 1/2 RESEND] greybus: raw: fix use-after-free on cdev close

by Damien Riégel

This addresses a use-after-free bug when a raw bundle is disconnected but its chardev is still opened by an application. When the application releases the cdev, it causes the following panic when init on free is enabled (CONFIG_INIT_ON_FREE_DEFAULT_ON=y): [ 78.451062] refcount_t: underflow; use-after-free. [ 78.451352] WARNING: CPU: 0 PID: 139 at lib/refcount.c:28 refcount_warn_saturate+0xd0/0x130 [ 78.451698] Modules linked in: gb_raw(C) [ 78.451881] CPU: 0 UID: 0 PID: 139 Comm: raw_chardev_tes Tainted: G WC 6.18.0-rc4 #212 PREEMPT(voluntary) [ 78.452386] Tainted: [W]=WARN, [C]=CRAP [ 78.452560] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.17.0-0-gb52ca86e094d-prebuilt.qemu.org 04/01/2014 [ 78.453049] RIP: 0010:refcount_warn_saturate+0xd0/0x130 [ 78.453311] Code: 0b 90 90 c3 cc cc cc cc 80 3d 4f ec 1d 01 00 0f 85 75 ff ff ff c6 05 42 ec 1d 01 01 90 48 c7 c7 e8 5b cb b4 e8 31f [ 78.453953] RSP: 0018:ffffaa0f80203ed0 EFLAGS: 00010282 [ 78.454251] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000 [ 78.454472] RDX: 0000000000000000 RSI: ffffaa0f80203d68 RDI: 00000000ffffdfff [ 78.454690] RBP: 00000000040e001f R08: 00000000ffffdfff R09: ffffffffb510c008 [ 78.454899] R10: ffffffffb505c060 R11: 0000000063666572 R12: ffff938dc210b468 [ 78.455279] R13: ffff938dc1f5e1a0 R14: ffff938dc14710c0 R15: 0000000000000000 [ 78.455549] FS: 00007f2f22741740(0000) GS:ffff938e11fbc000(0000) knlGS:0000000000000000 [ 78.455806] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 78.456129] CR2: 00007f2f228c89c3 CR3: 00000000020d0000 CR4: 00000000000006f0 [ 78.456786] Call Trace: [ 78.456936] <TASK> [ 78.457069] cdev_put+0x18/0x30 [ 78.457230] __fput+0x255/0x2a0 [ 78.457372] __x64_sys_close+0x3d/0x80 [ 78.457544] do_syscall_64+0xa4/0x290 [ 78.457697] entry_SYSCALL_64_after_hwframe+0x77/0x7f [ 78.457883] RIP: 0033:0x7f2f227d1cc7 [ 78.458097] Code: 48 89 fa 4c 89 df e8 08 ae 00 00 8b 93 08 03 00 00 59 5e 48 83 f8 fc 74 1a 5b c3 0f 1f 84 00 00 00 00 00 48 8b 44f [ 78.458692] RSP: 002b:00007fffab36fb50 EFLAGS: 00000202 ORIG_RAX: 0000000000000003 [ 78.459155] RAX: ffffffffffffffda RBX: 00007f2f22741740 RCX: 00007f2f227d1cc7 [ 78.459400] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000003 [ 78.459648] RBP: 00007fffab36fba8 R08: 0000000000000000 R09: 0000000000000000 [ 78.459899] R10: 0000000000000000 R11: 0000000000000202 R12: 0000558298427128 [ 78.460212] R13: 00007f2f227416d0 R14: 00005582c72cf320 R15: 00005582c72cf320 [ 78.460470] </TASK> [ 78.460571] ---[ end trace 0000000000000000 ]--- The cdev is contained in the "gb_raw" structure, which is freed in the disconnect operation. When the cdev is released at a later time, cdev_put gets an address that points to freed memory. To fix this use-after-free, convert the struct device from a pointer to being embedded, that makes the lifetime of the cdev and of this device the same. Then, use cdev_device_add, which guarantees that the device won't be released until all references to the cdev are not released. Finally, delegate the freeing of the structure to the device release function, instead of freeing immediately in the disconnect callback. Fixes: e806c7fb8e9b ("greybus: raw: add raw greybus kernel driver") Signed-off-by: Damien Riégel <damien.riegel(a)silabs.com> --- resend: added linux-staging as Cc, this list was not part of the first submission. drivers/staging/greybus/raw.c | 49 +++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/drivers/staging/greybus/raw.c b/drivers/staging/greybus/raw.c index 71de6776739..b92214f97e3 100644 --- a/drivers/staging/greybus/raw.c +++ b/drivers/staging/greybus/raw.c @@ -21,9 +21,8 @@ struct gb_raw { struct list_head list; int list_data; struct mutex list_lock; - dev_t dev; struct cdev cdev; - struct device *device; + struct device dev; }; struct raw_data { @@ -148,6 +147,13 @@ static int gb_raw_send(struct gb_raw *raw, u32 len, const char __user *data) return retval; } +static void raw_dev_release(struct device *dev) +{ + struct gb_raw *raw = dev_get_drvdata(dev); + + kfree(raw); +} + static int gb_raw_probe(struct gb_bundle *bundle, const struct greybus_bundle_id *id) { @@ -168,11 +174,14 @@ static int gb_raw_probe(struct gb_bundle *bundle, if (!raw) return -ENOMEM; + device_initialize(&raw->dev); + dev_set_drvdata(&raw->dev, raw); + connection = gb_connection_create(bundle, le16_to_cpu(cport_desc->id), gb_raw_request_handler); if (IS_ERR(connection)) { retval = PTR_ERR(connection); - goto error_free; + goto error_put_device; } INIT_LIST_HEAD(&raw->list); @@ -187,29 +196,26 @@ static int gb_raw_probe(struct gb_bundle *bundle, goto error_connection_destroy; } - raw->dev = MKDEV(raw_major, minor); + raw->dev.devt = MKDEV(raw_major, minor); + raw->dev.class = &raw_class; + raw->dev.parent = &connection->bundle->dev; + raw->dev.release = raw_dev_release; + retval = dev_set_name(&raw->dev, "gb!raw%d", minor); + if (retval) + goto error_remove_ida; + cdev_init(&raw->cdev, &raw_fops); retval = gb_connection_enable(connection); if (retval) goto error_remove_ida; - retval = cdev_add(&raw->cdev, raw->dev, 1); + retval = cdev_device_add(&raw->cdev, &raw->dev); if (retval) goto error_connection_disable; - raw->device = device_create(&raw_class, &connection->bundle->dev, - raw->dev, raw, "gb!raw%d", minor); - if (IS_ERR(raw->device)) { - retval = PTR_ERR(raw->device); - goto error_del_cdev; - } - return 0; -error_del_cdev: - cdev_del(&raw->cdev); - error_connection_disable: gb_connection_disable(connection); @@ -219,8 +225,8 @@ static int gb_raw_probe(struct gb_bundle *bundle, error_connection_destroy: gb_connection_destroy(connection); -error_free: - kfree(raw); +error_put_device: + put_device(&raw->dev); return retval; } @@ -231,11 +237,9 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) struct raw_data *raw_data; struct raw_data *temp; - // FIXME - handle removing a connection when the char device node is open. - device_destroy(&raw_class, raw->dev); - cdev_del(&raw->cdev); + cdev_device_del(&raw->cdev, &raw->dev); gb_connection_disable(connection); - ida_free(&minors, MINOR(raw->dev)); + ida_free(&minors, MINOR(raw->dev.devt)); gb_connection_destroy(connection); mutex_lock(&raw->list_lock); @@ -244,8 +248,7 @@ static void gb_raw_disconnect(struct gb_bundle *bundle) kfree(raw_data); } mutex_unlock(&raw->list_lock); - - kfree(raw); + put_device(&raw->dev); } /* -- 2.52.0

7 months, 1 week

[PATCH] greybus: Omit a redundant pm_runtime_mark_last_busy() call in two functions()

by Markus Elfring

From: Markus Elfring <elfring(a)users.sourceforge.net> Date: Sat, 14 Mar 2026 16:00:20 +0100 The device's last busy timestamp was set in a wrapper function since the commit 18c1fe53d186867243f4cf17f4eef60737a16c4c ("PM: runtime: Mark last busy stamp in pm_request_autosuspend()"). Thus delete a pm_runtime_mark_last_busy() call before two pm_request_autosuspend() calls. The source code was transformed by using the Coccinelle software. Signed-off-by: Markus Elfring <elfring(a)users.sourceforge.net> --- drivers/greybus/bundle.c | 1 - drivers/greybus/interface.c | 1 - 2 files changed, 2 deletions(-) diff --git a/drivers/greybus/bundle.c b/drivers/greybus/bundle.c index d1831d0986e9..d8d8e9ba7869 100644 --- a/drivers/greybus/bundle.c +++ b/drivers/greybus/bundle.c @@ -155,7 +155,6 @@ static int gb_bundle_resume(struct device *dev) static int gb_bundle_idle(struct device *dev) { - pm_runtime_mark_last_busy(dev); pm_request_autosuspend(dev); return 0; diff --git a/drivers/greybus/interface.c b/drivers/greybus/interface.c index 4ee4bda4a267..ed56f90369d1 100644 --- a/drivers/greybus/interface.c +++ b/drivers/greybus/interface.c @@ -753,7 +753,6 @@ static int gb_interface_resume(struct device *dev) static int gb_interface_runtime_idle(struct device *dev) { - pm_runtime_mark_last_busy(dev); pm_request_autosuspend(dev); return 0; -- 2.53.0

7 months, 1 week

[PATCH] staging: greybus: gbphy: Omit a redundant pm_runtime_mark_last_busy() call in gb_gbphy_idle()

by Markus Elfring

From: Markus Elfring <elfring(a)users.sourceforge.net> Date: Sat, 14 Mar 2026 15:35:09 +0100 The device's last busy timestamp was set in a wrapper function since the commit 18c1fe53d186867243f4cf17f4eef60737a16c4c ("PM: runtime: Mark last busy stamp in pm_request_autosuspend()"). Thus delete a pm_runtime_mark_last_busy() call before a pm_request_autosuspend() call. The source code was transformed by using the Coccinelle software. Signed-off-by: Markus Elfring <elfring(a)users.sourceforge.net> --- drivers/staging/greybus/gbphy.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/staging/greybus/gbphy.c b/drivers/staging/greybus/gbphy.c index bdb0f5164a6f..949656e75e8b 100644 --- a/drivers/staging/greybus/gbphy.c +++ b/drivers/staging/greybus/gbphy.c @@ -53,7 +53,6 @@ static void gbphy_dev_release(struct device *dev) #ifdef CONFIG_PM static int gb_gbphy_idle(struct device *dev) { - pm_runtime_mark_last_busy(dev); pm_request_autosuspend(dev); return 0; } -- 2.53.0

7 months, 1 week

[PATCH] greybus: svc: use kzalloc_flex

by Rosen Penev

Avoid manual sizeof math by using the proper helper. Also use struct_size for the buffer size. Signed-off-by: Rosen Penev <rosenp(a)gmail.com> --- drivers/greybus/svc.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/greybus/svc.c b/drivers/greybus/svc.c index 1b854f53f21e..490577731a19 100644 --- a/drivers/greybus/svc.c +++ b/drivers/greybus/svc.c @@ -775,10 +775,9 @@ static void gb_svc_pwrmon_debugfs_init(struct gb_svc *svc) if (!rail_count || rail_count > GB_SVC_PWRMON_MAX_RAIL_COUNT) goto err_pwrmon_debugfs; - bufsize = sizeof(*rail_names) + - GB_SVC_PWRMON_RAIL_NAME_BUFSIZE * rail_count; + bufsize = struct_size(rail_names, name, rail_count); - rail_names = kzalloc(bufsize, GFP_KERNEL); + rail_names = kzalloc_flex(*rail_names, name, rail_count); if (!rail_names) goto err_pwrmon_debugfs; -- 2.53.0

7 months, 1 week

[PATCH] staging: greybus: bootrom: fix potential null pointer dereference

by Oarora Etimis

In gb_bootrom_get_firmware(), the 'fw' pointer could be NULL if the function jumps to the 'unlock' label. The execution flow continues into the 'queue_work' block where 'fw->size' is accessed, leading to a null pointer dereference. Fix this by adding a NULL check for 'fw' before accessing its members. Signed-off-by: Oarora Etimis <OaroraEtimis(a)gmail.com> --- drivers/staging/greybus/bootrom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c index 83921d90c322..50c80475d241 100644 --- a/drivers/staging/greybus/bootrom.c +++ b/drivers/staging/greybus/bootrom.c @@ -298,7 +298,7 @@ static int gb_bootrom_get_firmware(struct gb_operation *op) queue_work: /* Refresh timeout */ - if (!ret && (offset + size == fw->size)) + if (!ret && fw && (offset + size == fw->size)) next_request = NEXT_REQ_READY_TO_BOOT; else next_request = NEXT_REQ_GET_FIRMWARE; -- 2.47.3

7 months, 1 week

[PATCH v2 2/2] staging: greybus: bootrom: fix potential null pointer dereference

by Oarora Etimis

In gb_bootrom_get_firmware(), the 'fw' pointer could be NULL if the function jumps to the 'unlock' label. The execution flow continues into the 'queue_work' block where 'fw->size' is accessed, leading to a null pointer dereference. Fix this by adding a NULL check for 'fw' before accessing its members. Signed-off-by: Oarora Etimis <OaroraEtimis(a)gmail.com> --- Changes in v2: - Rebased onto the latest staging-next branch to resolve merge conflicts. - No logical code changes. drivers/staging/greybus/bootrom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c index 83921d90c322..50c80475d241 100644 --- a/drivers/staging/greybus/bootrom.c +++ b/drivers/staging/greybus/bootrom.c @@ -298,7 +298,7 @@ static int gb_bootrom_get_firmware(struct gb_operation *op) queue_work: /* Refresh timeout */ - if (!ret && (offset + size == fw->size)) + if (!ret && fw && (offset + size == fw->size)) next_request = NEXT_REQ_READY_TO_BOOT; else next_request = NEXT_REQ_GET_FIRMWARE; -- 2.47.3

7 months, 1 week

[PATCH] staging: greybus: use %pe to print PTR_ERR in fw-core.c

by Tomasz Unger

Replace PTR_ERR() with %pe format specifier which directly prints the error pointer in a human readable way, making the code cleaner and more idiomatic. Signed-off-by: Tomasz Unger <tomasz.unger(a)yahoo.pl> --- Verified with checkpatch.pl - no errors or warnings. Compiled the gb-firmware module successfully. Module compiles and loads in a QEMU environment. --- drivers/staging/greybus/fw-core.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/staging/greybus/fw-core.c b/drivers/staging/greybus/fw-core.c index 2016a74f137f..7053afa9ab3e 100644 --- a/drivers/staging/greybus/fw-core.c +++ b/drivers/staging/greybus/fw-core.c @@ -112,8 +112,8 @@ static int gb_fw_core_probe(struct gb_bundle *bundle, connection = gb_connection_create(bundle, cport_id, gb_fw_download_request_handler); if (IS_ERR(connection)) { - dev_err(&bundle->dev, "failed to create download connection (%ld)\n", - PTR_ERR(connection)); + dev_err(&bundle->dev, "failed to create download connection (%pe)\n", + connection); } else { fw_core->download_connection = connection; } @@ -131,8 +131,8 @@ static int gb_fw_core_probe(struct gb_bundle *bundle, connection = gb_connection_create(bundle, cport_id, NULL); if (IS_ERR(connection)) { - dev_err(&bundle->dev, "failed to create SPI connection (%ld)\n", - PTR_ERR(connection)); + dev_err(&bundle->dev, "failed to create SPI connection (%pe)\n", + connection); } else { fw_core->spi_connection = connection; } @@ -149,8 +149,8 @@ static int gb_fw_core_probe(struct gb_bundle *bundle, connection = gb_connection_create(bundle, cport_id, NULL); if (IS_ERR(connection)) { - dev_err(&bundle->dev, "failed to create Authentication connection (%ld)\n", - PTR_ERR(connection)); + dev_err(&bundle->dev, "failed to create Authentication connection (%pe)\n", + connection); } else { fw_core->cap_connection = connection; } --- base-commit: ad6bb64332bb4297110950769ad5af52791e33a2 change-id: 20260314-greybus-pe-format-85993705ee22 Best regards, -- Tomasz Unger <tomasz.unger(a)yahoo.pl>

7 months, 1 week

[PATCH] staging: greybus: uart: add comments to locks and fix alignment

by Rahul Joshi

The spinlock_t and struct mutex members in struct gb_tty lack comments describing what they protect, which is required by the kernel coding style. Also fix the alignment of the wait_for_completion_timeout() call in gb_uart_wait_for_all_credits() to match the open parenthesis. Signed-off-by: Rahul Joshi <rj5547884(a)gmail.com> --- drivers/staging/greybus/uart.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c index 7d060b4cd33d..52a84a68049c 100644 --- a/drivers/staging/greybus/uart.c +++ b/drivers/staging/greybus/uart.c @@ -50,12 +50,12 @@ struct gb_tty { unsigned int minor; unsigned char clocal; bool disconnected; - spinlock_t read_lock; - spinlock_t write_lock; + spinlock_t read_lock; /* protects iocount and oldcount */ + spinlock_t write_lock; /* protects write_fifo and credits */ struct async_icount iocount; struct async_icount oldcount; wait_queue_head_t wioctl; - struct mutex mutex; + struct mutex mutex; /* protects disconnected flag and device state */ u8 ctrlin; /* input control lines */ u8 ctrlout; /* output control lines */ struct gb_uart_set_line_coding_request line_coding; @@ -318,7 +318,7 @@ static int gb_uart_wait_for_all_credits(struct gb_tty *gb_tty) return 0; ret = wait_for_completion_timeout(&gb_tty->credits_complete, - msecs_to_jiffies(GB_UART_CREDIT_WAIT_TIMEOUT_MSEC)); + msecs_to_jiffies(GB_UART_CREDIT_WAIT_TIMEOUT_MSEC)); if (!ret) { dev_err(&gb_tty->gbphy_dev->dev, "time out waiting for credits\n"); -- 2.34.1

7 months, 1 week

[PATCH] staging: greybus: loopback: remove unused argument from macro

by Giacomo Di Clerico

The gb_dev_loopback_ro_attr macro accepted a 'conn' argument which was never used in its expansion. Remove it from both the macro definition and its invocation. Signed-off-by: Giacomo Di Clerico <giacomodiclerico(a)gmail.com> --- drivers/staging/greybus/loopback.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c index aa9c73cb0ae5..855d5051c55d 100644 --- a/drivers/staging/greybus/loopback.c +++ b/drivers/staging/greybus/loopback.c @@ -193,7 +193,7 @@ static ssize_t field##_store(struct device *dev, \ } \ static DEVICE_ATTR_RW(field) -#define gb_dev_loopback_ro_attr(field, conn) \ +#define gb_dev_loopback_ro_attr(field) \ static ssize_t field##_show(struct device *dev, \ struct device_attribute *attr, \ char *buf) \ @@ -305,7 +305,7 @@ gb_dev_loopback_rw_attr(us_wait, d); /* Maximum iterations for a given operation: 1-(2^32-1), 0 implies infinite */ gb_dev_loopback_rw_attr(iteration_max, u); /* The current index of the for (i = 0; i < iteration_max; i++) loop */ -gb_dev_loopback_ro_attr(iteration_count, false); +gb_dev_loopback_ro_attr(iteration_count); /* A flag to indicate synchronous or asynchronous operations */ gb_dev_loopback_rw_attr(async, u); /* Timeout of an individual asynchronous request */ -- 2.53.0

7 months, 1 week

[PATCH] staging: greybus: uart: replace DEFINE_IDR with DEFINE_XARRAY_ALLOC

by Rahul Joshi

DEFINE_IDR is deprecated in favour of DEFINE_XARRAY_ALLOC. Replace the tty_minors IDR with an XArray and update all call sites: idr_alloc() -> xa_alloc() idr_find() -> xa_load() idr_remove() -> xa_erase() idr_destroy() -> xa_destroy() Also remove the now-unused <linux/idr.h> include and add <linux/xarray.h>. Signed-off-by: Rahul Joshi <rj5547884(a)gmail.com> --- drivers/staging/greybus/uart.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c index 7d060b4cd33d..bed3f35a6ec2 100644 --- a/drivers/staging/greybus/uart.c +++ b/drivers/staging/greybus/uart.c @@ -22,7 +22,7 @@ #include <linux/serial.h> #include <linux/tty_driver.h> #include <linux/tty_flip.h> -#include <linux/idr.h> +#include <linux/xarray.h> #include <linux/fs.h> #include <linux/kdev_t.h> #include <linux/kfifo.h> @@ -67,7 +67,7 @@ struct gb_tty { }; static struct tty_driver *gb_tty_driver; -static DEFINE_IDR(tty_minors); +static DEFINE_XARRAY_ALLOC(tty_minors); static DEFINE_MUTEX(table_lock); static int gb_uart_receive_data_handler(struct gb_operation *op) @@ -342,7 +342,7 @@ static struct gb_tty *get_gb_by_minor(unsigned int minor) struct gb_tty *gb_tty; mutex_lock(&table_lock); - gb_tty = idr_find(&tty_minors, minor); + gb_tty = xa_load(&tty_minors, minor); if (gb_tty) { mutex_lock(&gb_tty->mutex); if (gb_tty->disconnected) { @@ -359,14 +359,18 @@ static struct gb_tty *get_gb_by_minor(unsigned int minor) static int alloc_minor(struct gb_tty *gb_tty) { - int minor; + int retval; + u32 index; mutex_lock(&table_lock); - minor = idr_alloc(&tty_minors, gb_tty, 0, GB_NUM_MINORS, GFP_KERNEL); + retval = xa_alloc(&tty_minors, &index, gb_tty, + XA_LIMIT(0, GB_NUM_MINORS - 1), GFP_KERNEL); mutex_unlock(&table_lock); - if (minor >= 0) - gb_tty->minor = minor; - return minor; + if (retval < 0) + return retval; + + gb_tty->minor = index; + return index; } static void release_minor(struct gb_tty *gb_tty) @@ -375,7 +379,7 @@ static void release_minor(struct gb_tty *gb_tty) gb_tty->minor = 0; /* Maybe should use an invalid value instead */ mutex_lock(&table_lock); - idr_remove(&tty_minors, minor); + xa_erase(&tty_minors, minor); mutex_unlock(&table_lock); } @@ -984,7 +988,7 @@ static void gb_tty_exit(void) { tty_unregister_driver(gb_tty_driver); tty_driver_kref_put(gb_tty_driver); - idr_destroy(&tty_minors); + xa_destroy(&tty_minors); } static const struct gbphy_device_id gb_uart_id_table[] = { -- 2.34.1

7 months, 2 weeks

← Newer
1
...
18
19
20
21
22
23
24
...
91
Older →

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

greybus-dev