greybus-dev

greybus-dev@lists.linaro.org

12 participants
815 discussions

[PATCH] greybus: manifest: validate string descriptor header

by Pengpeng Hou

identify_descriptor() computes a string descriptor size from desc->string.length. Require the descriptor to contain the fixed string descriptor header before reading the variable string length. The existing descriptor-size check then reports the short descriptor. Signed-off-by: Pengpeng Hou <pengpeng(a)iscas.ac.cn> --- drivers/greybus/manifest.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/greybus/manifest.c b/drivers/greybus/manifest.c index 9be5d95da587..3b685ace2036 100644 --- a/drivers/greybus/manifest.c +++ b/drivers/greybus/manifest.c @@ -122,6 +122,8 @@ static int identify_descriptor(struct gb_interface *intf, switch (desc_header->type) { case GREYBUS_TYPE_STRING: expected_size += sizeof(struct greybus_descriptor_string); + if (desc_size < expected_size) + break; expected_size += desc->string.length; /* String descriptors are padded to 4 byte boundaries */ -- 2.43.0

1 week, 2 days

[PATCH] staging: greybus: bootrom: power off module on timeout

by adi25charis＠gmail.com

From: Aditya Chari S <adi25charis(a)gmail.com> Previously, when a firmware size request, get firmware request, or ready-to-boot request from a module timed out, the driver would log an error and free any firmware buffer, but left the module powered on in a potentially inconsistent state. Fix this by ejecting the module's interface via the SVC when a timeout occurs, forcing it to be power-cycled and re-detected rather than left in limbo. Signed-off-by: Aditya Chari S <adi25charis(a)gmail.com> --- drivers/staging/greybus/bootrom.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c index 83921d90c..b564d2255 100644 --- a/drivers/staging/greybus/bootrom.c +++ b/drivers/staging/greybus/bootrom.c @@ -54,8 +54,12 @@ static void gb_bootrom_timedout(struct work_struct *work) struct delayed_work *dwork = to_delayed_work(work); struct gb_bootrom *bootrom = container_of(dwork, struct gb_bootrom, dwork); - struct device *dev = &bootrom->connection->bundle->dev; + struct gb_connection *connection = bootrom->connection; + struct device *dev = &connection->bundle->dev; + struct gb_interface *intf = connection->bundle->intf; + struct gb_svc *svc = connection->hd->svc; const char *reason; + int ret; switch (bootrom->next_request) { case NEXT_REQ_FIRMWARE_SIZE: @@ -82,7 +86,11 @@ static void gb_bootrom_timedout(struct work_struct *work) free_firmware(bootrom); mutex_unlock(&bootrom->mutex); - /* TODO: Power-off Module ? */ + /* Power-off Module */ + ret = gb_svc_intf_eject(svc, intf->interface_id); + if (ret) + dev_err(dev, "failed to eject interface %u (%d)\n", + intf->interface_id, ret); } static void gb_bootrom_set_timeout(struct gb_bootrom *bootrom, -- 2.53.0

1 week, 4 days

[PATCH] staging: greybus: add comments to mutex declarations

by suryasaimadhu

Add comments to mutex members in gbaudio_codec_info struct to describe what each mutex protects, as recommended by checkpatch. Signed-off-by: suryasaimadhu <suryasaimadhu369(a)gmail.com> --- drivers/staging/greybus/audio_codec.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h index f3f7a7ec6..326770b99 100644 --- a/drivers/staging/greybus/audio_codec.h +++ b/drivers/staging/greybus/audio_codec.h @@ -70,8 +70,8 @@ struct gbaudio_codec_info { struct list_head module_list; /* to maintain runtime stream params for each DAI */ struct list_head dai_list; - struct mutex lock; - struct mutex register_mutex; + struct mutex lock; /* protects module_list and dai_list */ + struct mutex register_mutex; /* protects module registration */ }; struct gbaudio_widget { -- 2.47.3

2 weeks, 6 days

[PATCH] staging: greybus: add missing newlines in sysfs_emit calls

by suryasaimadhu

Add missing terminating newlines to sysfs_emit format strings in audio_manager_module.c as required by sysfs ABI. Signed-off-by: suryasaimadhu <suryasaimadhu369(a)gmail.com> --- drivers/staging/greybus/audio_manager_module.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/staging/greybus/audio_manager_module.c b/drivers/staging/greybus/audio_manager_module.c index dc90cc2d2..5737f2a32 100644 --- a/drivers/staging/greybus/audio_manager_module.c +++ b/drivers/staging/greybus/audio_manager_module.c @@ -75,7 +75,7 @@ static void gb_audio_module_release(struct kobject *kobj) static ssize_t gb_audio_module_name_show(struct gb_audio_manager_module *module, struct gb_audio_manager_module_attribute *attr, char *buf) { - return sysfs_emit(buf, "%s", module->desc.name); + return sysfs_emit(buf, "%s\n", module->desc.name); } static struct gb_audio_manager_module_attribute gb_audio_module_name_attribute = @@ -84,7 +84,7 @@ static struct gb_audio_manager_module_attribute gb_audio_module_name_attribute = static ssize_t gb_audio_module_vid_show(struct gb_audio_manager_module *module, struct gb_audio_manager_module_attribute *attr, char *buf) { - return sysfs_emit(buf, "%d", module->desc.vid); + return sysfs_emit(buf, "%d\n", module->desc.vid); } static struct gb_audio_manager_module_attribute gb_audio_module_vid_attribute = @@ -93,7 +93,7 @@ static struct gb_audio_manager_module_attribute gb_audio_module_vid_attribute = static ssize_t gb_audio_module_pid_show(struct gb_audio_manager_module *module, struct gb_audio_manager_module_attribute *attr, char *buf) { - return sysfs_emit(buf, "%d", module->desc.pid); + return sysfs_emit(buf, "%d\n", module->desc.pid); } static struct gb_audio_manager_module_attribute gb_audio_module_pid_attribute = @@ -103,7 +103,7 @@ static ssize_t gb_audio_module_intf_id_show(struct gb_audio_manager_module *modu struct gb_audio_manager_module_attribute *attr, char *buf) { - return sysfs_emit(buf, "%d", module->desc.intf_id); + return sysfs_emit(buf, "%d\n", module->desc.intf_id); } static struct gb_audio_manager_module_attribute @@ -114,7 +114,7 @@ static ssize_t gb_audio_module_ip_devices_show(struct gb_audio_manager_module *m struct gb_audio_manager_module_attribute *attr, char *buf) { - return sysfs_emit(buf, "0x%X", module->desc.ip_devices); + return sysfs_emit(buf, "0x%X\n", module->desc.ip_devices); } static struct gb_audio_manager_module_attribute @@ -125,7 +125,7 @@ static ssize_t gb_audio_module_op_devices_show(struct gb_audio_manager_module *m struct gb_audio_manager_module_attribute *attr, char *buf) { - return sysfs_emit(buf, "0x%X", module->desc.op_devices); + return sysfs_emit(buf, "0x%X\n", module->desc.op_devices); } static struct gb_audio_manager_module_attribute -- 2.47.3

2 weeks, 6 days

staging: greybus: audio: possible out of bounds read in the topology parser

by Maoyi Xie

Hi all, I think the Greybus audio topology parser in drivers/staging/greybus/audio_topology.c can read past the topology blob when a module reports inconsistent counts. I would appreciate it if you could take a look. gb_audio_gb_get_topology() allocates the blob to a device-reported u16 size and only checks that it is at least sizeof(struct gb_audio_topology). size = le16_to_cpu(size_resp.size); if (size < sizeof(*topo)) return -ENODATA; topo = kzalloc(size, GFP_KERNEL); After that the parser trusts the interior fields. gb_generate_enum_strings() walks one C string per item with no end pointer. items = le32_to_cpu(gbenum->items); for (i = 0; i < items; i++) { strings[i] = data; while (*data != '\0') data++; data++; } items is a device le32 and there is no guarantee the blob holds that many NUL terminated strings, so the while loop runs off the end of the allocation. The same pattern is in gbaudio_tplg_process_header(), which derives the control, widget and route offsets by adding device le32 block sizes with no check against the end of the blob, and in the csize cursor advance in gbaudio_tplg_process_kcontrols() and gbaudio_tplg_create_widget(). The whole blob is device data parsed on the probe path with no privilege needed. The attacker here is a malicious or faulty Greybus audio module on the interface. I reproduced the enum string walk under KASAN on 7.1-rc7. A blob with one NUL per item stays inside the allocation. A blob with missing terminators makes KASAN report a slab out of bounds read past the buffer. I have a partial fix that passes the buffer end into the enum walk and stops on overrun. A complete fix also needs to bound the header offsets and the csize advances against the allocation size, so I wanted to ask before sending a series. Does this look like a real bug to you? If it does I am happy to send a proper patch. The parser was added in c8e6336bb3f8 ("greybus: audio: Add topology parser for GB codec"), which I think is the right Fixes tag. Thanks, Maoyi https://maoyixie.com/

3 weeks, 6 days

[PATCH v2] staging: greybus: vibrator: return device_create() errors

by Alfie Varghese

gb_vibrator_probe() maps any device_create() failure to -EINVAL. This loses the real errno returned by the driver core, such as -ENOMEM, and makes probe failures harder to diagnose correctly. Return PTR_ERR(dev) instead so callers receive the actual failure reason while preserving the existing cleanup path. Signed-off-by: Alfie Varghese <alfievarghese22(a)gmail.com> --- v2: No code changes, resending as a single properly versioned patch. drivers/staging/greybus/vibrator.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/greybus/vibrator.c b/drivers/staging/greybus/vibrator.c index 0ec4d317c9db..763c234fbc03 100644 --- a/drivers/staging/greybus/vibrator.c +++ b/drivers/staging/greybus/vibrator.c @@ -161,7 +161,7 @@ static int gb_vibrator_probe(struct gb_bundle *bundle, dev = device_create(&vibrator_class, &bundle->dev, MKDEV(0, 0), vib, "vibrator%d", vib->minor); if (IS_ERR(dev)) { - retval = -EINVAL; + retval = PTR_ERR(dev); goto err_ida_remove; } vib->dev = dev; -- 2.53.0

3 weeks, 6 days

[PATCH] staging: greybus: vibrator: return device_create() errors

by Alfie Varghese

gb_vibrator_probe() maps any device_create() failure to -EINVAL. This loses the real errno returned by the driver core, such as -ENOMEM, and makes probe failures harder to diagnose correctly. Return PTR_ERR(dev) instead so callers receive the actual failure reason while preserving the existing cleanup path. Signed-off-by: Alfie Varghese <alfievarghese22(a)gmail.com> --- drivers/staging/greybus/vibrator.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/greybus/vibrator.c b/drivers/staging/greybus/vibrator.c index 0ec4d317c..763c234fb 100644 --- a/drivers/staging/greybus/vibrator.c +++ b/drivers/staging/greybus/vibrator.c @@ -161,7 +161,7 @@ static int gb_vibrator_probe(struct gb_bundle *bundle, dev = device_create(&vibrator_class, &bundle->dev, MKDEV(0, 0), vib, "vibrator%d", vib->minor); if (IS_ERR(dev)) { - retval = -EINVAL; + retval = PTR_ERR(dev); goto err_ida_remove; } vib->dev = dev; -- 2.54.0.windows.1

3 weeks, 6 days

[PATCH] staging: greybus: power_supply: remove unnecessary parentheses in return

by Tomasz Unger

return is not a function, so the parentheses around the returned expressions are not needed. Remove them to follow kernel coding style and silence checkpatch.pl RETURN_PARENTHESES warnings. Verified with checkpatch.pl - no errors or warnings. Compiled the power_supply object successfully. Signed-off-by: Tomasz Unger <tomasz.unger(a)yahoo.pl> --- drivers/staging/greybus/power_supply.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/greybus/power_supply.c b/drivers/staging/greybus/power_supply.c index 33b7a63979d6..44bd8a72fa50 100644 --- a/drivers/staging/greybus/power_supply.c +++ b/drivers/staging/greybus/power_supply.c @@ -336,7 +336,7 @@ static int is_psy_prop_writeable(struct gb_power_supply *gbpsy, static int is_prop_valint(enum power_supply_property psp) { - return ((psp < POWER_SUPPLY_PROP_MODEL_NAME) ? 1 : 0); + return (psp < POWER_SUPPLY_PROP_MODEL_NAME) ? 1 : 0; } static void next_interval(struct gb_power_supply *gbpsy) @@ -423,7 +423,7 @@ static void check_changed(struct gb_power_supply *gbpsy, static int total_props(struct gb_power_supply *gbpsy) { /* this return the intval plus the strval properties */ - return (gbpsy->properties_count + gbpsy->properties_count_str); + return gbpsy->properties_count + gbpsy->properties_count_str; } static void prop_append(struct gb_power_supply *gbpsy, --- base-commit: 7cb1c5b32a2bfde961fff8d5204526b609bcb30a change-id: 20260615-greybus-power-supply-return-parens-b3dd9f4a87c7 Best regards, -- Tomasz Unger <tomasz.unger(a)yahoo.pl>

4 weeks, 1 day

[PATCH] staging: greybus: uart: replace IDR with XArray

by Jack Lee

DEFINE_IDR is deprecated in favor of XArray. Convert tty_minors from IDR to XArray, replacing idr_alloc, idr_find, idr_remove and idr_destroy with their xa_alloc, xa_load, xa_erase and xa_destroy equivalents. Signed-off-by: Jack Lee <skunkolee(a)gmail.com> --- drivers/staging/greybus/uart.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c index 7d060b4cd33d..30afcd0caa14 100644 --- a/drivers/staging/greybus/uart.c +++ b/drivers/staging/greybus/uart.c @@ -22,13 +22,13 @@ #include <linux/serial.h> #include <linux/tty_driver.h> #include <linux/tty_flip.h> -#include <linux/idr.h> #include <linux/fs.h> #include <linux/kdev_t.h> #include <linux/kfifo.h> #include <linux/workqueue.h> #include <linux/completion.h> #include <linux/greybus.h> +#include <linux/xarray.h> #include "gbphy.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,13 @@ static struct gb_tty *get_gb_by_minor(unsigned int minor) static int alloc_minor(struct gb_tty *gb_tty) { - int minor; + int ret; - mutex_lock(&table_lock); - minor = idr_alloc(&tty_minors, gb_tty, 0, GB_NUM_MINORS, GFP_KERNEL); - mutex_unlock(&table_lock); - if (minor >= 0) - gb_tty->minor = minor; - return minor; + ret = xa_alloc(&tty_minors, &gb_tty->minor, gb_tty, + XA_LIMIT(0, GB_NUM_MINORS - 1), GFP_KERNEL); + if (ret) + return ret; + return gb_tty->minor; } static void release_minor(struct gb_tty *gb_tty) @@ -375,7 +374,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 +983,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.54.0

4 weeks, 1 day

[PATCH] greybus: audio: bound the topology section sizes against the fetched size

by Bryam Vargas via B4 Relay

From: Bryam Vargas <hexlabsecurity(a)proton.me> gb_audio_gb_get_topology() fetches a topology blob of a module-supplied size, and gbaudio_tplg_parse_data() then walks it by adding the module-supplied size_dais, size_controls and size_widgets fields to form the control, widget and route section offsets. Those le32 sizes are never checked against the fetched blob, so a module reporting a small topology size but large section sizes makes the offsets point past the allocation, and parsing reads out of bounds. Reject a topology whose section sizes do not fit within the fetched size before it is parsed. Fixes: 184992e305f1 ("greybus: audio: Add Greybus Audio Device Class Protocol helper routines") Cc: stable(a)vger.kernel.org Signed-off-by: Bryam Vargas <hexlabsecurity(a)proton.me> --- I reproduced the out-of-bounds read both in-kernel under KASAN and with a userspace AddressSanitizer model of the gbaudio_tplg_process_header() offset walk. The topology blob is kzalloc(size) where size is module-supplied (a u16), and process_header() forms control_offset = &data + size_dais, widget_offset = control_offset + size_controls, etc.; the consumers then read structs at those offsets. - In-kernel (7.1.0-rc5 + KASAN): a 64-byte blob (header 24, so 40 bytes available) with size_dais = 44 makes control_offset point 4 bytes past the allocation, and reading the first control byte there trips: BUG: KASAN: slab-out-of-bounds in ...parse_topology... Read of size 1 at addr ... ... which belongs to the cache kmalloc-64 of size 64 The buggy address is located 4 bytes to the right of allocated 64-byte region The patched arm (sections rejected, -EINVAL) and an in-bounds control arm (size_dais = 8) read cleanly with no KASAN report. - ASan model (-m32 and -m64): size_dais = 4096 makes control_offset point ~4 KB past the 64-byte blob - heap-buffer-overflow READ located 4056 bytes after the region, both ABIs; patched and in-bounds clean. The source is a greybus audio module trust boundary (an attacker-supplied or compromised module reporting a malformed topology); the access is a read, and a large size_dais sends the offset far enough to fault. The reproducer (kernel module + ASan model) is available on request. --- drivers/staging/greybus/audio_gb.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/drivers/staging/greybus/audio_gb.c b/drivers/staging/greybus/audio_gb.c index 9d8994fdb41a..144591f1a512 100644 --- a/drivers/staging/greybus/audio_gb.c +++ b/drivers/staging/greybus/audio_gb.c @@ -37,6 +37,19 @@ int gb_audio_gb_get_topology(struct gb_connection *connection, return ret; } + /* + * The size_* fields are supplied by the module and are used by + * gbaudio_tplg_parse_data() to compute offsets into the blob; make + * sure the sections fit within the fetched topology, so walking it + * cannot read out of bounds. + */ + if ((u64)le32_to_cpu(topo->size_dais) + le32_to_cpu(topo->size_controls) + + le32_to_cpu(topo->size_widgets) + le32_to_cpu(topo->size_routes) > + size - sizeof(*topo)) { + kfree(topo); + return -EINVAL; + } + *topology = topo; return 0; --- base-commit: 8e65320d91cdc3b241d4b94855c88459b91abf66 change-id: 20260616-b4-disp-4352e8b0-45e86659956e Best regards, -- Bryam Vargas <hexlabsecurity(a)proton.me>

1 month

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

greybus-dev