greybus-dev

greybus-dev@lists.linaro.org

6 participants
859 discussions

[PATCH v4] greybus: audio: Split gb_audio_gb_get_topology() into size query and data fetch

by Aditya Chari

`gb_audio_gb_get_topology()` combined three separate responsibilities into a single call: querying the topology size, allocating a buffer for it, and fetching the topology data into that buffer. This left callers with no way to perform any of these steps independently, and forced the kzalloc() allocation to live inside the protocol‑layer driver rather than the caller, as already flagged by a FIXME comment at the call site in `audio_module.c`. Split the function into two: - `gb_audio_gb_get_topology_size()` – queries only the topology size - `gb_audio_gb_get_topology()` – fetches topology data into a caller‑supplied buffer of a given size Update the only caller, `gb_audio_probe()` in `audio_module.c`, to query the size first, allocate the topology buffer itself, then fetch the data into it, freeing the buffer via the existing `free_topology` error path on failure. The topology size is now stored as `size_t` and validated in the caller before allocation, addressing the earlier TODO and FIXME comments. This resolves both the "TODO: Split into separate calls" comment above the original function in `audio_gb.c` and the FIXME comment at the call site in `audio_module.c`, both of which are removed as part of this change. No functional change in behavior for the existing probe path. Signed-off-by: Aditya Chari S <adi25charis(a)gmail.com> Reviewed-by: Dan Carpenter <error27(a)gmail.com> --- v4: - Reorder tags (Signed-off-by before Reviewed-by) as suggested by Dan. v3: - Rebased against latest staging-next. v2: - Fold in review feedback from Dan Carpenter. - Store topology size as size_t instead of u16. - Move topology size validation into gb_audio_probe() before kzalloc(). - Use -EINVAL for invalid topology size. - Drop unrelated dev_err() formatting cleanup. - Compile-tested with `make M=drivers/staging/greybus`. - Run checkpatch.pl on the updated patch; no issues reported. --- drivers/staging/greybus/audio_codec.h | 4 ++- drivers/staging/greybus/audio_gb.c | 45 +++++++------------------- drivers/staging/greybus/audio_module.c | 27 ++++++++++++---- 3 files changed, 35 insertions(+), 41 deletions(-) diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h index f3f7a7ec6..b45cd257d 100644 --- a/drivers/staging/greybus/audio_codec.h +++ b/drivers/staging/greybus/audio_codec.h @@ -178,8 +178,10 @@ int gbaudio_register_module(struct gbaudio_module_info *module); void gbaudio_unregister_module(struct gbaudio_module_info *module); /* protocol related */ +int gb_audio_gb_get_topology_size(struct gb_connection *connection, + size_t *size); int gb_audio_gb_get_topology(struct gb_connection *connection, - struct gb_audio_topology **topology); + struct gb_audio_topology *topology, size_t size); int gb_audio_gb_get_control(struct gb_connection *connection, u8 control_id, u8 index, struct gb_audio_ctl_elem_value *value); diff --git a/drivers/staging/greybus/audio_gb.c b/drivers/staging/greybus/audio_gb.c index 144591f1a..2e6f155d8 100644 --- a/drivers/staging/greybus/audio_gb.c +++ b/drivers/staging/greybus/audio_gb.c @@ -8,13 +8,10 @@ #include <linux/greybus.h> #include "audio_codec.h" -/* TODO: Split into separate calls */ -int gb_audio_gb_get_topology(struct gb_connection *connection, - struct gb_audio_topology **topology) +int gb_audio_gb_get_topology_size(struct gb_connection *connection, + size_t *size) { struct gb_audio_get_topology_size_response size_resp; - struct gb_audio_topology *topo; - u16 size; int ret; ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE, @@ -22,38 +19,18 @@ int gb_audio_gb_get_topology(struct gb_connection *connection, if (ret) return ret; - size = le16_to_cpu(size_resp.size); - if (size < sizeof(*topo)) - return -ENODATA; - - topo = kzalloc(size, GFP_KERNEL); - if (!topo) - return -ENOMEM; - - ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY, NULL, 0, - topo, size); - if (ret) { - kfree(topo); - 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; + *size = le16_to_cpu(size_resp.size); return 0; } +EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology_size); + +int gb_audio_gb_get_topology(struct gb_connection *connection, + struct gb_audio_topology *topology, size_t size) +{ + return gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY, NULL, 0, + topology, size); +} EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology); int gb_audio_gb_get_control(struct gb_connection *connection, diff --git a/drivers/staging/greybus/audio_module.c b/drivers/staging/greybus/audio_module.c index 12c376c47..4cd1f42c1 100644 --- a/drivers/staging/greybus/audio_module.c +++ b/drivers/staging/greybus/audio_module.c @@ -239,6 +239,7 @@ static int gb_audio_probe(struct gb_bundle *bundle, struct gb_audio_manager_module_descriptor desc; struct gbaudio_data_connection *dai, *_dai; int ret, i; + size_t size; struct gb_audio_topology *topology; /* There should be at least one Management and one Data cport */ @@ -304,16 +305,30 @@ static int gb_audio_probe(struct gb_bundle *bundle, } gbmodule->dev_id = gbmodule->mgmt_connection->intf->interface_id; - /* - * FIXME: malloc for topology happens via audio_gb driver - * should be done within codec driver itself - */ - ret = gb_audio_gb_get_topology(gbmodule->mgmt_connection, &topology); + ret = gb_audio_gb_get_topology_size(gbmodule->mgmt_connection, &size); if (ret) { - dev_err(dev, "%d:Error while fetching topology\n", ret); + dev_err(dev, "%d:Error while fetching topology size\n", ret); + goto disable_connection; + } + + if (size < sizeof(*topology)) { + dev_err(dev, "Invalid topology size: %zu\n", size); + ret = -EINVAL; goto disable_connection; } + topology = kzalloc(size, GFP_KERNEL); + if (!topology) { + ret = -ENOMEM; + goto disable_connection; + } + + ret = gb_audio_gb_get_topology(gbmodule->mgmt_connection, topology, size); + if (ret) { + dev_err(dev, "%d:Error while fetching topology\n", ret); + goto free_topology; + } + /* process topology data */ ret = gbaudio_tplg_parse_data(gbmodule, topology); if (ret) { -- 2.53.0

1 month, 1 week

[PATCH v3] greybus: audio: Split gb_audio_gb_get_topology() into size query and data fetch

by Aditya Chari

`gb_audio_gb_get_topology()` combined three separate responsibilities into a single call: querying the topology size, allocating a buffer for it, and fetching the topology data into that buffer. This left callers with no way to perform any of these steps independently, and forced the kzalloc() allocation to live inside the protocol‑layer driver rather than the caller, as already flagged by a FIXME comment at the call site in `audio_module.c`. Split the function into two: - `gb_audio_gb_get_topology_size()` – queries only the topology size - `gb_audio_gb_get_topology()` – fetches topology data into a caller‑supplied buffer of a given size Update the only caller, `gb_audio_probe()` in `audio_module.c`, to query the size first, allocate the topology buffer itself, then fetch the data into it, freeing the buffer via the existing `free_topology` error path on failure. The topology size is now stored as `size_t` and validated in the caller before allocation, addressing the earlier TODO and FIXME comments. This resolves both the "TODO: Split into separate calls" comment above the original function in `audio_gb.c` and the FIXME comment at the call site in `audio_module.c`, both of which are removed as part of this change. No functional change in behavior for the existing probe path. Reviewed-by: Dan Carpenter <error27(a)gmail.com> Signed-off-by: Aditya Chari <adi25charis(a)gmail.com> --- drivers/staging/greybus/audio_codec.h | 4 ++- drivers/staging/greybus/audio_gb.c | 45 +++++++------------------- drivers/staging/greybus/audio_module.c | 27 ++++++++++++---- 3 files changed, 35 insertions(+), 41 deletions(-) diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h index f3f7a7ec6..b45cd257d 100644 --- a/drivers/staging/greybus/audio_codec.h +++ b/drivers/staging/greybus/audio_codec.h @@ -178,8 +178,10 @@ int gbaudio_register_module(struct gbaudio_module_info *module); void gbaudio_unregister_module(struct gbaudio_module_info *module); /* protocol related */ +int gb_audio_gb_get_topology_size(struct gb_connection *connection, + size_t *size); int gb_audio_gb_get_topology(struct gb_connection *connection, - struct gb_audio_topology **topology); + struct gb_audio_topology *topology, size_t size); int gb_audio_gb_get_control(struct gb_connection *connection, u8 control_id, u8 index, struct gb_audio_ctl_elem_value *value); diff --git a/drivers/staging/greybus/audio_gb.c b/drivers/staging/greybus/audio_gb.c index 144591f1a..2e6f155d8 100644 --- a/drivers/staging/greybus/audio_gb.c +++ b/drivers/staging/greybus/audio_gb.c @@ -8,13 +8,10 @@ #include <linux/greybus.h> #include "audio_codec.h" -/* TODO: Split into separate calls */ -int gb_audio_gb_get_topology(struct gb_connection *connection, - struct gb_audio_topology **topology) +int gb_audio_gb_get_topology_size(struct gb_connection *connection, + size_t *size) { struct gb_audio_get_topology_size_response size_resp; - struct gb_audio_topology *topo; - u16 size; int ret; ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE, @@ -22,38 +19,18 @@ int gb_audio_gb_get_topology(struct gb_connection *connection, if (ret) return ret; - size = le16_to_cpu(size_resp.size); - if (size < sizeof(*topo)) - return -ENODATA; - - topo = kzalloc(size, GFP_KERNEL); - if (!topo) - return -ENOMEM; - - ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY, NULL, 0, - topo, size); - if (ret) { - kfree(topo); - 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; + *size = le16_to_cpu(size_resp.size); return 0; } +EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology_size); + +int gb_audio_gb_get_topology(struct gb_connection *connection, + struct gb_audio_topology *topology, size_t size) +{ + return gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY, NULL, 0, + topology, size); +} EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology); int gb_audio_gb_get_control(struct gb_connection *connection, diff --git a/drivers/staging/greybus/audio_module.c b/drivers/staging/greybus/audio_module.c index 12c376c47..4cd1f42c1 100644 --- a/drivers/staging/greybus/audio_module.c +++ b/drivers/staging/greybus/audio_module.c @@ -239,6 +239,7 @@ static int gb_audio_probe(struct gb_bundle *bundle, struct gb_audio_manager_module_descriptor desc; struct gbaudio_data_connection *dai, *_dai; int ret, i; + size_t size; struct gb_audio_topology *topology; /* There should be at least one Management and one Data cport */ @@ -304,16 +305,30 @@ static int gb_audio_probe(struct gb_bundle *bundle, } gbmodule->dev_id = gbmodule->mgmt_connection->intf->interface_id; - /* - * FIXME: malloc for topology happens via audio_gb driver - * should be done within codec driver itself - */ - ret = gb_audio_gb_get_topology(gbmodule->mgmt_connection, &topology); + ret = gb_audio_gb_get_topology_size(gbmodule->mgmt_connection, &size); if (ret) { - dev_err(dev, "%d:Error while fetching topology\n", ret); + dev_err(dev, "%d:Error while fetching topology size\n", ret); + goto disable_connection; + } + + if (size < sizeof(*topology)) { + dev_err(dev, "Invalid topology size: %zu\n", size); + ret = -EINVAL; goto disable_connection; } + topology = kzalloc(size, GFP_KERNEL); + if (!topology) { + ret = -ENOMEM; + goto disable_connection; + } + + ret = gb_audio_gb_get_topology(gbmodule->mgmt_connection, topology, size); + if (ret) { + dev_err(dev, "%d:Error while fetching topology\n", ret); + goto free_topology; + } + /* process topology data */ ret = gbaudio_tplg_parse_data(gbmodule, topology); if (ret) { -- 2.53.0

1 month, 1 week

[PATCH 0/7] staging: Remove redundant error messages on IRQ request failure

by Pan Chuang

devm_request_threaded_irq() and devm_request_any_context_irq() automatically log detailed error messages on failure via the devm_request_result() helper, which prints device name, IRQ number, handler functions, and error code. Since devm_request_irq() is a static inline wrapper around devm_request_threaded_irq(), it also benefits from this automatic logging. Remove the now-redundant dev_err() and dev_err_probe() calls in staging drivers that follow these devm_request_*_irq() functions, as the core now provides more detailed diagnostic information on failure. Pan Chuang (7): staging: axis-fifo: Remove redundant dev_err() staging: fbtft: Remove redundant dev_err_probe() staging: greybus: arche-platform: Remove redundant dev_err() staging: iio: adt7316: Remove redundant dev_err() media: atomisp: Remove redundant dev_err() staging: most: dim2: Remove redundant dev_err() staging: nvec: Remove redundant dev_err_probe() drivers/staging/axis-fifo/axis-fifo.c | 5 +---- drivers/staging/fbtft/fb_st7789v.c | 2 +- drivers/staging/greybus/arche-platform.c | 4 +--- drivers/staging/iio/addac/adt7316.c | 5 +---- drivers/staging/media/atomisp/pci/atomisp_v4l2.c | 4 +--- drivers/staging/most/dim2/dim2.c | 8 ++------ drivers/staging/nvec/nvec.c | 2 +- 7 files changed, 8 insertions(+), 22 deletions(-) -- 2.34.1

1 month, 2 weeks

[PATCH] staging: greybus: audio: split topology get into size and data calls

by adi25charis＠gmail.com

From: Aditya Chari S <adi25charis(a)gmail.com> gb_audio_gb_get_topology() combined three separate responsibilities into a single call: querying the topology size, allocating a buffer for it, and fetching the topology data into that buffer. This left callers with no way to perform any of these steps independently, and forced the kzalloc() allocation to live inside the protocol-layer driver rather than the caller, as already flagged by a FIXME comment at the call site in audio_module.c. Split the function into two: gb_audio_gb_get_topology_size() - queries only the topology size gb_audio_gb_get_topology() - fetches topology data into a caller-supplied buffer of a given size Update the only caller, gb_audio_probe() in audio_module.c, to query the size first, allocate the topology buffer itself, then fetch the data into it, freeing the buffer via the existing free_topology error path on failure. This resolves both the "TODO: Split into separate calls" comment above the original function in audio_gb.c and the FIXME comment at the call site in audio_module.c, both of which are removed as part of this change. No functional change in behavior for the existing probe path. Compile-tested with W=1, sparse (C=2), and checkpatch.pl; all clean on the three changed files (audio_gb.c, audio_module.c, audio_codec.h). Signed-off-by: Aditya Chari S <adi25charis(a)gmail.com> --- drivers/staging/greybus/audio_codec.h | 4 +++- drivers/staging/greybus/audio_gb.c | 33 ++++++++++---------------- drivers/staging/greybus/audio_module.c | 21 +++++++++++----- 3 files changed, 31 insertions(+), 27 deletions(-) diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h index f3f7a7ec6..be5a2a86b 100644 --- a/drivers/staging/greybus/audio_codec.h +++ b/drivers/staging/greybus/audio_codec.h @@ -178,8 +178,10 @@ int gbaudio_register_module(struct gbaudio_module_info *module); void gbaudio_unregister_module(struct gbaudio_module_info *module); /* protocol related */ +int gb_audio_gb_get_topology_size(struct gb_connection *connection, + u16 *size); int gb_audio_gb_get_topology(struct gb_connection *connection, - struct gb_audio_topology **topology); + struct gb_audio_topology *topology, u16 size); int gb_audio_gb_get_control(struct gb_connection *connection, u8 control_id, u8 index, struct gb_audio_ctl_elem_value *value); diff --git a/drivers/staging/greybus/audio_gb.c b/drivers/staging/greybus/audio_gb.c index 9d8994fdb..e6356643d 100644 --- a/drivers/staging/greybus/audio_gb.c +++ b/drivers/staging/greybus/audio_gb.c @@ -8,13 +8,10 @@ #include <linux/greybus.h> #include "audio_codec.h" -/* TODO: Split into separate calls */ -int gb_audio_gb_get_topology(struct gb_connection *connection, - struct gb_audio_topology **topology) +int gb_audio_gb_get_topology_size(struct gb_connection *connection, + u16 *size) { struct gb_audio_get_topology_size_response size_resp; - struct gb_audio_topology *topo; - u16 size; int ret; ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE, @@ -22,24 +19,20 @@ int gb_audio_gb_get_topology(struct gb_connection *connection, if (ret) return ret; - size = le16_to_cpu(size_resp.size); - if (size < sizeof(*topo)) - return -ENODATA; - - topo = kzalloc(size, GFP_KERNEL); - if (!topo) - return -ENOMEM; + *size = le16_to_cpu(size_resp.size); - ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY, NULL, 0, - topo, size); - if (ret) { - kfree(topo); - return ret; - } + return 0; +} +EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology_size); - *topology = topo; +int gb_audio_gb_get_topology(struct gb_connection *connection, + struct gb_audio_topology *topology, u16 size) +{ + if (size < sizeof(*topology)) + return -ENODATA; - return 0; + return gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY, NULL, 0, + topology, size); } EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology); diff --git a/drivers/staging/greybus/audio_module.c b/drivers/staging/greybus/audio_module.c index 12c376c47..1163cf093 100644 --- a/drivers/staging/greybus/audio_module.c +++ b/drivers/staging/greybus/audio_module.c @@ -239,6 +239,7 @@ static int gb_audio_probe(struct gb_bundle *bundle, struct gb_audio_manager_module_descriptor desc; struct gbaudio_data_connection *dai, *_dai; int ret, i; + u16 size; struct gb_audio_topology *topology; /* There should be at least one Management and one Data cport */ @@ -304,16 +305,24 @@ static int gb_audio_probe(struct gb_bundle *bundle, } gbmodule->dev_id = gbmodule->mgmt_connection->intf->interface_id; - /* - * FIXME: malloc for topology happens via audio_gb driver - * should be done within codec driver itself - */ - ret = gb_audio_gb_get_topology(gbmodule->mgmt_connection, &topology); + ret = gb_audio_gb_get_topology_size(gbmodule->mgmt_connection, &size); if (ret) { - dev_err(dev, "%d:Error while fetching topology\n", ret); + dev_err(dev, "%d:Error while fetching topology size\n", ret); + goto disable_connection; + } + + topology = kzalloc(size, GFP_KERNEL); + if (!topology) { + ret = -ENOMEM; goto disable_connection; } + ret = gb_audio_gb_get_topology(gbmodule->mgmt_connection, topology, size); + if (ret) { + dev_err(dev, "%d:Error while fetching topology\n", ret); + goto free_topology; + } + /* process topology data */ ret = gbaudio_tplg_parse_data(gbmodule, topology); if (ret) { -- 2.53.0

1 month, 2 weeks

[PATCH] staging: greybus: audio: bound topology parsing to the received buffer

by Muhammad Bilal

The Greybus audio topology parser trusts length and count fields taken straight from the module's topology blob and never checks them against the size of the buffer it actually allocated, leading to out-of-bounds reads of the kernel heap. gb_audio_gb_get_topology() reads a u16 size from the module, allocates a buffer of that size, fetches the topology into it, and then discards the size. gbaudio_tplg_parse_data() then walks that buffer using fields stored inside it: - gbaudio_tplg_process_header() computes the control, widget and route block offsets by adding the wire-supplied __le32 size_dais, size_controls and size_widgets onto the buffer base with no bound, so a module that reports a small allocation size but large block sizes moves those offsets far past the end of the buffer before they are dereferenced. - gbaudio_tplg_process_kcontrols(), _process_widgets() and _process_routes() iterate num_controls / num_widgets / num_routes (also from the blob) and advance a pointer by a per-element size that includes the __le16 names_length of an enumerated control, again with no check that the element stays inside the buffer. - gb_generate_enum_strings() loops over an attacker-controlled __le32 items count and, for each, scans for a NUL terminator with no end pointer, walking off the end of the buffer. A malicious or malfunctioning module can therefore make the parser read past the allocation. The wild block offsets are most likely to hit an unmapped page and oops (denial of service); the byte-at-a-time enum scan walks from a still-valid pointer and can copy adjacent heap bytes into ALSA control name strings, which are readable by unprivileged local users, so an information leak cannot be ruled out. Thread the allocated topology size from gb_audio_gb_get_topology() through to gbaudio_tplg_parse_data() and bound every walk against the end of the buffer: verify the block offsets are ordered and within the buffer (the "< previous" tests also catch a 32-bit unsigned wrap of the running offset), check each control, widget and route lies fully inside its block before use, and give gb_generate_enum_strings() an explicit end pointer plus an items-versus-names_length sanity check. Fixes: 6339d2322c47 ("greybus: audio: Add topology parser for GB codec") Cc: stable(a)vger.kernel.org Signed-off-by: Muhammad Bilal <meatuni001(a)gmail.com> --- drivers/staging/greybus/audio_codec.h | 4 +- drivers/staging/greybus/audio_gb.c | 13 ++- drivers/staging/greybus/audio_module.c | 6 +- drivers/staging/greybus/audio_topology.c | 135 ++++++++++++++++++----- 4 files changed, 120 insertions(+), 38 deletions(-) diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h index f3f7a7ec6be40..f9225cf52d843 100644 --- a/drivers/staging/greybus/audio_codec.h +++ b/drivers/staging/greybus/audio_codec.h @@ -167,7 +167,7 @@ struct gbaudio_module_info { }; int gbaudio_tplg_parse_data(struct gbaudio_module_info *module, - struct gb_audio_topology *tplg_data); + struct gb_audio_topology *tplg_data, size_t size); void gbaudio_tplg_release(struct gbaudio_module_info *module); int gbaudio_module_update(struct gbaudio_codec_info *codec, @@ -179,7 +179,7 @@ void gbaudio_unregister_module(struct gbaudio_module_info *module); /* protocol related */ int gb_audio_gb_get_topology(struct gb_connection *connection, - struct gb_audio_topology **topology); + struct gb_audio_topology **topology, size_t *size); int gb_audio_gb_get_control(struct gb_connection *connection, u8 control_id, u8 index, struct gb_audio_ctl_elem_value *value); diff --git a/drivers/staging/greybus/audio_gb.c b/drivers/staging/greybus/audio_gb.c index 9d8994fdb41a2..0811652bc417f 100644 --- a/drivers/staging/greybus/audio_gb.c +++ b/drivers/staging/greybus/audio_gb.c @@ -10,11 +10,11 @@ /* TODO: Split into separate calls */ int gb_audio_gb_get_topology(struct gb_connection *connection, - struct gb_audio_topology **topology) + struct gb_audio_topology **topology, size_t *size) { struct gb_audio_get_topology_size_response size_resp; struct gb_audio_topology *topo; - u16 size; + u16 tplg_size; int ret; ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE, @@ -22,22 +22,23 @@ int gb_audio_gb_get_topology(struct gb_connection *connection, if (ret) return ret; - size = le16_to_cpu(size_resp.size); - if (size < sizeof(*topo)) + tplg_size = le16_to_cpu(size_resp.size); + if (tplg_size < sizeof(*topo)) return -ENODATA; - topo = kzalloc(size, GFP_KERNEL); + topo = kzalloc(tplg_size, GFP_KERNEL); if (!topo) return -ENOMEM; ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY, NULL, 0, - topo, size); + topo, tplg_size); if (ret) { kfree(topo); return ret; } *topology = topo; + *size = tplg_size; return 0; } diff --git a/drivers/staging/greybus/audio_module.c b/drivers/staging/greybus/audio_module.c index 12c376c477b3c..9367ab6debdbe 100644 --- a/drivers/staging/greybus/audio_module.c +++ b/drivers/staging/greybus/audio_module.c @@ -240,6 +240,7 @@ static int gb_audio_probe(struct gb_bundle *bundle, struct gbaudio_data_connection *dai, *_dai; int ret, i; struct gb_audio_topology *topology; + size_t tplg_size; /* There should be at least one Management and one Data cport */ if (bundle->num_cports < 2) @@ -308,14 +309,15 @@ static int gb_audio_probe(struct gb_bundle *bundle, * FIXME: malloc for topology happens via audio_gb driver * should be done within codec driver itself */ - ret = gb_audio_gb_get_topology(gbmodule->mgmt_connection, &topology); + ret = gb_audio_gb_get_topology(gbmodule->mgmt_connection, &topology, + &tplg_size); if (ret) { dev_err(dev, "%d:Error while fetching topology\n", ret); goto disable_connection; } /* process topology data */ - ret = gbaudio_tplg_parse_data(gbmodule, topology); + ret = gbaudio_tplg_parse_data(gbmodule, topology, tplg_size); if (ret) { dev_err(dev, "%d:Error while parsing topology data\n", ret); diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c index 76146f91cddcc..4095e6c741efa 100644 --- a/drivers/staging/greybus/audio_topology.c +++ b/drivers/staging/greybus/audio_topology.c @@ -134,21 +134,35 @@ static const char **gb_generate_enum_strings(struct gbaudio_module_info *gb, struct gb_audio_enumerated *gbenum) { const char **strings; - int i; unsigned int items; - __u8 *data; + u16 names_length; + const __u8 *data; + const __u8 *end; + int i; items = le32_to_cpu(gbenum->items); + names_length = le16_to_cpu(gbenum->names_length); + data = gbenum->names; + end = data + names_length; + + /* + * Each enumerated value is a NUL-terminated string occupying at least + * one byte, so a valid names block cannot hold more items than it has + * bytes. This also bounds the devm_kcalloc() request below. + */ + if (items > names_length) + return NULL; + strings = devm_kcalloc(gb->dev, items, sizeof(char *), GFP_KERNEL); if (!strings) return NULL; - data = gbenum->names; - for (i = 0; i < items; i++) { strings[i] = (const char *)data; - while (*data != '\0') + while (data < end && *data != '\0') data++; + if (data == end) + return NULL; data++; } @@ -1009,9 +1023,40 @@ static const struct snd_soc_dapm_widget gbaudio_widgets[] = { SND_SOC_DAPM_POST_PMD), }; +/* + * Return the on-wire size of the topology control at @curr, in bytes, after + * verifying that the whole control - including its variable-length enum names + * block - lies within [@curr, @end). Returns a negative errno on overrun. + */ +static int gbaudio_control_size(struct gb_audio_control *curr, const u8 *end) +{ + size_t csize; + + /* + * Enough of the control must be present to read its id and name and + * the fixed part of the enumerated descriptor (items, names_length). + */ + csize = offsetof(struct gb_audio_control, info); + csize += offsetof(struct gb_audio_ctl_elem_info, value); + csize += offsetof(struct gb_audio_enumerated, names); + if ((u8 *)curr + csize > end) + return -EINVAL; + + if (curr->info.type == GB_AUDIO_CTL_ELEM_TYPE_ENUMERATED) + csize += le16_to_cpu(curr->info.value.enumerated.names_length); + else + csize = sizeof(struct gb_audio_control); + + if ((u8 *)curr + csize > end) + return -EINVAL; + + return csize; +} + static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module, struct snd_soc_dapm_widget *dw, - struct gb_audio_widget *w, int *w_size) + struct gb_audio_widget *w, int *w_size, + const u8 *end) { int i, ret, csize; struct snd_kcontrol_new *widget_kctls; @@ -1040,6 +1085,11 @@ static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module, /* create relevant kcontrols */ curr = w->ctl; for (i = 0; i < w->ncontrols; i++) { + ret = gbaudio_control_size(curr, end); + if (ret < 0) + goto error; + csize = ret; + ret = gbaudio_tplg_create_wcontrol(module, &widget_kctls[i], curr); if (ret) { @@ -1063,10 +1113,6 @@ static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module, struct gb_audio_enumerated *gbenum = &curr->info.value.enumerated; - csize = offsetof(struct gb_audio_control, info); - csize += offsetof(struct gb_audio_ctl_elem_info, value); - csize += offsetof(struct gb_audio_enumerated, names); - csize += le16_to_cpu(gbenum->names_length); control->texts = (const char * const *) gb_generate_enum_strings(module, gbenum); if (!control->texts) { @@ -1074,8 +1120,6 @@ static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module, goto error; } control->items = le32_to_cpu(gbenum->items); - } else { - csize = sizeof(struct gb_audio_control); } *w_size += csize; @@ -1136,7 +1180,8 @@ static int gbaudio_tplg_create_widget(struct gbaudio_module_info *module, } static int gbaudio_tplg_process_kcontrols(struct gbaudio_module_info *module, - struct gb_audio_control *controls) + struct gb_audio_control *controls, + const u8 *end) { int i, csize, ret; struct snd_kcontrol_new *dapm_kctls; @@ -1152,6 +1197,11 @@ static int gbaudio_tplg_process_kcontrols(struct gbaudio_module_info *module, curr = controls; for (i = 0; i < module->num_controls; i++) { + ret = gbaudio_control_size(curr, end); + if (ret < 0) + goto error; + csize = ret; + ret = gbaudio_tplg_create_kcontrol(module, &dapm_kctls[i], curr); if (ret) { @@ -1176,10 +1226,6 @@ static int gbaudio_tplg_process_kcontrols(struct gbaudio_module_info *module, struct gb_audio_enumerated *gbenum = &curr->info.value.enumerated; - csize = offsetof(struct gb_audio_control, info); - csize += offsetof(struct gb_audio_ctl_elem_info, value); - csize += offsetof(struct gb_audio_enumerated, names); - csize += le16_to_cpu(gbenum->names_length); control->texts = (const char * const *) gb_generate_enum_strings(module, gbenum); if (!control->texts) { @@ -1187,8 +1233,6 @@ static int gbaudio_tplg_process_kcontrols(struct gbaudio_module_info *module, goto error; } control->items = le32_to_cpu(gbenum->items); - } else { - csize = sizeof(struct gb_audio_control); } list_add(&control->list, &module->ctl_list); @@ -1210,7 +1254,8 @@ static int gbaudio_tplg_process_kcontrols(struct gbaudio_module_info *module, } static int gbaudio_tplg_process_widgets(struct gbaudio_module_info *module, - struct gb_audio_widget *widgets) + struct gb_audio_widget *widgets, + const u8 *end) { int i, ret, w_size; struct snd_soc_dapm_widget *dapm_widgets; @@ -1225,8 +1270,13 @@ static int gbaudio_tplg_process_widgets(struct gbaudio_module_info *module, curr = widgets; for (i = 0; i < module->num_dapm_widgets; i++) { + /* The fixed part of the widget must lie within the buffer. */ + if ((u8 *)curr + sizeof(struct gb_audio_widget) > end) { + ret = -EINVAL; + goto error; + } ret = gbaudio_tplg_create_widget(module, &dapm_widgets[i], - curr, &w_size); + curr, &w_size, end); if (ret) { dev_err(module->dev, "%s:%d type not supported\n", curr->name, curr->type); @@ -1259,7 +1309,8 @@ static int gbaudio_tplg_process_widgets(struct gbaudio_module_info *module, } static int gbaudio_tplg_process_routes(struct gbaudio_module_info *module, - struct gb_audio_route *routes) + struct gb_audio_route *routes, + const u8 *end) { int i, ret; struct snd_soc_dapm_route *dapm_routes; @@ -1275,6 +1326,10 @@ static int gbaudio_tplg_process_routes(struct gbaudio_module_info *module, curr = routes; for (i = 0; i < module->num_dapm_routes; i++) { + if ((u8 *)curr + sizeof(struct gb_audio_route) > end) { + ret = -EINVAL; + goto error; + } dapm_routes->sink = gbaudio_map_widgetid(module, curr->destination_id); if (!dapm_routes->sink) { @@ -1320,8 +1375,12 @@ static int gbaudio_tplg_process_routes(struct gbaudio_module_info *module, } static int gbaudio_tplg_process_header(struct gbaudio_module_info *module, - struct gb_audio_topology *tplg_data) + struct gb_audio_topology *tplg_data, + size_t size) { + unsigned long tplg_start = (unsigned long)tplg_data; + unsigned long tplg_end = tplg_start + size; + /* fetch no. of kcontrols, widgets & routes */ module->num_controls = tplg_data->num_controls; module->num_dapm_widgets = tplg_data->num_widgets; @@ -1336,6 +1395,20 @@ static int gbaudio_tplg_process_header(struct gbaudio_module_info *module, module->route_offset = module->widget_offset + le32_to_cpu(tplg_data->size_widgets); + /* + * The DAI, control, widget and route blocks are concatenated in that + * order after the header. Their sizes come straight off the wire and + * are attacker-controlled, so verify the resulting block boundaries + * are ordered and stay within the allocated topology buffer. The + * "< previous" tests also reject an unsigned wrap of the running + * offset on 32-bit builds. + */ + if (module->control_offset < module->dai_offset || + module->widget_offset < module->control_offset || + module->route_offset < module->widget_offset || + module->route_offset > tplg_end) + return -EINVAL; + dev_dbg(module->dev, "DAI offset is 0x%lx\n", module->dai_offset); dev_dbg(module->dev, "control offset is %lx\n", module->control_offset); @@ -1346,7 +1419,7 @@ static int gbaudio_tplg_process_header(struct gbaudio_module_info *module, } int gbaudio_tplg_parse_data(struct gbaudio_module_info *module, - struct gb_audio_topology *tplg_data) + struct gb_audio_topology *tplg_data, size_t size) { int ret; struct gb_audio_control *controls; @@ -1357,7 +1430,10 @@ int gbaudio_tplg_parse_data(struct gbaudio_module_info *module, if (!tplg_data) return -EINVAL; - ret = gbaudio_tplg_process_header(module, tplg_data); + if (size < sizeof(*tplg_data)) + return -EINVAL; + + ret = gbaudio_tplg_process_header(module, tplg_data, size); if (ret) { dev_err(module->dev, "%d: Error in parsing topology header\n", ret); @@ -1366,7 +1442,8 @@ int gbaudio_tplg_parse_data(struct gbaudio_module_info *module, /* process control */ controls = (struct gb_audio_control *)module->control_offset; - ret = gbaudio_tplg_process_kcontrols(module, controls); + ret = gbaudio_tplg_process_kcontrols(module, controls, + (const u8 *)module->widget_offset); if (ret) { dev_err(module->dev, "%d: Error in parsing controls data\n", ret); @@ -1376,7 +1453,8 @@ int gbaudio_tplg_parse_data(struct gbaudio_module_info *module, /* process widgets */ widgets = (struct gb_audio_widget *)module->widget_offset; - ret = gbaudio_tplg_process_widgets(module, widgets); + ret = gbaudio_tplg_process_widgets(module, widgets, + (const u8 *)module->route_offset); if (ret) { dev_err(module->dev, "%d: Error in parsing widgets data\n", ret); @@ -1386,7 +1464,8 @@ int gbaudio_tplg_parse_data(struct gbaudio_module_info *module, /* process route */ routes = (struct gb_audio_route *)module->route_offset; - ret = gbaudio_tplg_process_routes(module, routes); + ret = gbaudio_tplg_process_routes(module, routes, + (const u8 *)tplg_data + size); if (ret) { dev_err(module->dev, "%d: Error in parsing routes data\n", ret); -- 2.55.0

1 month, 2 weeks

[PATCH] staging: greybus: Replace sprintf with sysfs_emit

by Mohammad Davoudi

Replace sprintf() with sysfs_emit() in sysfs show functions. The sprintf() function is unaware of the PAGE_SIZE limit of the sysfs buffer, which can potentially lead to buffer overruns. sysfs_emit() is strictly designed for sysfs attributes and inherently protects against buffer boundaries. Signed-off-by: Mohammad Davoudi <MDavoudi2011(a)gmail.com> --- drivers/staging/greybus/gbphy.c | 2 +- drivers/staging/greybus/light.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/staging/greybus/gbphy.c b/drivers/staging/greybus/gbphy.c index bdb0f5164..bb9a5b538 100644 --- a/drivers/staging/greybus/gbphy.c +++ b/drivers/staging/greybus/gbphy.c @@ -31,7 +31,7 @@ static ssize_t protocol_id_show(struct device *dev, { struct gbphy_device *gbphy_dev = to_gbphy_dev(dev); - return sprintf(buf, "0x%02x\n", gbphy_dev->cport_desc->protocol_id); + return sysfs_emit(buf, "0x%02x\n", gbphy_dev->cport_desc->protocol_id); } static DEVICE_ATTR_RO(protocol_id); diff --git a/drivers/staging/greybus/light.c b/drivers/staging/greybus/light.c index cab02b5da..2689f9a75 100644 --- a/drivers/staging/greybus/light.c +++ b/drivers/staging/greybus/light.c @@ -173,7 +173,7 @@ static ssize_t fade_##__dir##_show(struct device *dev, \ struct led_classdev *cdev = dev_get_drvdata(dev); \ struct gb_channel *channel = get_channel_from_cdev(cdev); \ \ - return sprintf(buf, "%u\n", channel->fade_##__dir); \ + return sysfs_emit(buf, "%u\n", channel->fade_##__dir); \ } \ \ static ssize_t fade_##__dir##_store(struct device *dev, \ @@ -220,7 +220,7 @@ static ssize_t color_show(struct device *dev, struct device_attribute *attr, struct led_classdev *cdev = dev_get_drvdata(dev); struct gb_channel *channel = get_channel_from_cdev(cdev); - return sprintf(buf, "0x%08x\n", channel->color); + return sysfs_emit(buf, "0x%08x\n", channel->color); } static ssize_t color_store(struct device *dev, struct device_attribute *attr, -- 2.43.0

1 month, 2 weeks

[PATCH v3] staging: greybus: uart: return tty_alloc_driver() errors

by Alfie Varghese

gb_tty_init() maps any tty_alloc_driver() failure to -ENOMEM. tty_alloc_driver() currently always returns -ENOMEM on failure, so this does not change behavior in practice. However, returning PTR_ERR(gb_tty_driver) is more correct and consistent with kernel conventions, preserving any future error codes the function might return. Signed-off-by: Alfie Varghese <alfievarghese22(a)gmail.com> --- v3: resend, v2 was corrupted in transit. No code changes. v2: updated commit message per Dan Carpenter's review to clarify that tty_alloc_driver() currently only returns -ENOMEM, making this a style fix rather than a behavioral change. --- drivers/staging/greybus/uart.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c index 7d060b4cd33d..24b4dab069c3 100644 --- a/drivers/staging/greybus/uart.c +++ b/drivers/staging/greybus/uart.c @@ -951,7 +951,7 @@ static int gb_tty_init(void) TTY_DRIVER_DYNAMIC_DEV); if (IS_ERR(gb_tty_driver)) { pr_err("Can not allocate tty driver\n"); - retval = -ENOMEM; + retval = PTR_ERR(gb_tty_driver); goto fail_unregister_dev; } -- 2.54.0.windows.1

1 month, 2 weeks

[PATCH v2] staging: greybus: uart: return tty_alloc_driver() errors

by Alfie Varghese

gb_tty_init() maps any tty_alloc_driver() failure to -ENOMEM. tty_alloc_driver() currently always returns -ENOMEM on failure, so this does not change behavior in practice. However, returning PTR_ERR(gb_tty_driver) is more correct and consistent with kernel conventions, preserving any future error codes the function might return. Signed-off-by: Alfie Varghese <alfievarghese22(a)gmail.com> --- v2: updated commit message per Dan Carpenter's review to clarify that tty_alloc_driver() currently only returns -ENOMEM, making this a style fix rather than a behavioral change. drivers/staging/greybus/uart.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c index 7d060b4cd33d..24b4dab069c3 100644 --- a/drivers/staging/greybus/uart.c +++ b/drivers/staging/greybus/uart.c @@ -951,7 +951,7 @@ static int gb_tty_init(void) if (IS_ERR(gb_tty_driver)) { pr_err("Can not allocate tty driver\n"); - retval = -ENOMEM; + retval = PTR_ERR(gb_tty_driver); goto fail_unregister_dev; }

1 month, 2 weeks

[PATCH] staging: greybus: uart: return tty_alloc_driver() errors

by Alfie Varghese

gb_tty_init() maps any tty_alloc_driver() failure to -ENOMEM. This loses the real errno returned by the driver core and makes failures harder to diagnose correctly. Return PTR_ERR(gb_tty_driver) 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/uart.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c index 7d060b4cd33d..24b4dab069c3 100644 --- a/drivers/staging/greybus/uart.c +++ b/drivers/staging/greybus/uart.c @@ -951,7 +951,7 @@ static int gb_tty_init(void) TTY_DRIVER_DYNAMIC_DEV); if (IS_ERR(gb_tty_driver)) { pr_err("Can not allocate tty driver\n"); - retval = -ENOMEM; + retval = PTR_ERR(gb_tty_driver); goto fail_unregister_dev; } -- 2.54.0.windows.1

1 month, 2 weeks

[PATCH] staging: greybus: hid: fix SET_REPORT return value

by Hao-Qun Huang

__gb_hid_output_raw_report() stores the result of gb_hid_set_report() in ret and even adjusts it to account for the report ID byte, but then always returns 0. This hides Greybus transport errors from HID_REQ_SET_REPORT callers, and makes hidraw report zero bytes written to user space on success, although hid_hw_raw_request() is expected to return the number of bytes transferred or a negative errno. The sibling GET_REPORT path, __gb_hid_get_raw_report(), already follows this convention. Return ret like the other HID transport drivers do. Fixes: 96eab779e198 ("greybus: hid: add HID class driver") Cc: stable(a)vger.kernel.org Assisted-by: Claude:claude-fable-5 Signed-off-by: Hao-Qun Huang <alvinhuang0603(a)gmail.com> --- diff --git a/drivers/staging/greybus/hid.c b/drivers/staging/greybus/hid.c index f1f9f6fbc00e..1d7186eecd23 100644 --- a/drivers/staging/greybus/hid.c +++ b/drivers/staging/greybus/hid.c @@ -256,7 +256,7 @@ static int __gb_hid_output_raw_report(struct hid_device *hid, __u8 *buf, if (report_id && ret >= 0) ret++; /* add report_id to the number of transferred bytes */ - return 0; + return ret; } static int gb_hid_raw_request(struct hid_device *hid, unsigned char reportnum,

1 month, 3 weeks

← Newer
1
2
3
4
5
6
7
8
...
86
Older →

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

greybus-dev