* [PATCH 0/3] staging: vchiq: Fix vchiq_read return value in case of error
@ 2019-11-20 15:36 Marcelo Diop-Gonzalez
2019-11-20 15:36 ` [PATCH 1/3] staging: vchiq_dump: Replace min with min_t Marcelo Diop-Gonzalez
` (4 more replies)
0 siblings, 5 replies; 16+ messages in thread
From: Marcelo Diop-Gonzalez @ 2019-11-20 15:36 UTC (permalink / raw)
To: gregkh, eric, wahrenst; +Cc: devel, linux-rpi-kernel
This is a proposed fix of an issue regarding the handling of positive
return values from copy_to_user() in vchiq_read(). It looks like the
struct dump_context passed to the vchiq_dump() function keeps track
of the number of bytes written in the context->actual field. When
copy_to_user() returns a positive value, this is set to -EFAULT. The
problem is that this is never returned to the user, and instead the function
continues, adding the number of bytes that should have been copied
to context->actual.
Running the following program:
#include <stdio.h>
#include <stdlib.h>
#include <fcntl.h>
#include <unistd.h>
#include <errno.h>
int main(int argc, char **argv) {
int fd = open("/dev/vchiq", 0);
if (fd < 0)
exit(-1);
sbrk(getpagesize());
char *bad_buf = sbrk(0)-100;
int x = read(fd, bad_buf, 2000);
printf("%d %d\n", x, errno);
puts(bad_buf);
}
gives this output:
-1 1
State 0: CONNECTED
tx_po
Remote: slots 2-32 tx_pos=578 recycle=1f
Slots claimed:
Note the mangled output and incorrect errno value. Messing with the
constants in that toy program changes the results. Sometimes read()
returns with no error, sometimes it returns with a wrong error code,
sometimes with the right one. But it seems that it only ever returns an
error at all accidentally, due to the fact that the comparison between
context->actual and context->space in vchiq_dump() is unsigned, so that
that function won't do anything else if it ever sets context->actual
to a negative value.
After this patchset, the above program prints this:
-1 14
State 0: CONNECTED
tx_pos=b4a218(@165de6b4), rx_pos=ae0668(@f02b54f4)
Version: 8 (min 3)
Stats
Help with testing would be appreciated. So far I've basically just
diffed the output of 'cat /dev/vchiq', run the program above with
a few different values, and run vchiq_test a few times.
These were applied to the staging-next branch of the tree
at git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
Marcelo Diop-Gonzalez (3):
staging: vchiq_dump: Replace min with min_t
staging: vchiq: Refactor indentation in vchiq_dump_* functions
staging: vchiq: Have vchiq_dump_* functions return an error code
.../interface/vchiq_arm/vchiq_2835_arm.c | 5 +-
.../interface/vchiq_arm/vchiq_arm.c | 129 ++++++++++--------
.../interface/vchiq_arm/vchiq_core.c | 104 ++++++++++----
.../interface/vchiq_arm/vchiq_core.h | 12 +-
4 files changed, 153 insertions(+), 97 deletions(-)
--
2.20.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] staging: vchiq_dump: Replace min with min_t
2019-11-20 15:36 [PATCH 0/3] staging: vchiq: Fix vchiq_read return value in case of error Marcelo Diop-Gonzalez
@ 2019-11-20 15:36 ` Marcelo Diop-Gonzalez
2019-11-20 15:46 ` Greg KH
2019-11-20 15:36 ` [PATCH 2/3] staging: vchiq: Refactor indentation in vchiq_dump_* functions Marcelo Diop-Gonzalez
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Marcelo Diop-Gonzalez @ 2019-11-20 15:36 UTC (permalink / raw)
To: gregkh, eric, wahrenst; +Cc: devel, linux-rpi-kernel
Replacing this and fixing the block comment format in this
function fixes checkpatch warnings.
Signed-off-by: Marcelo Diop-Gonzalez <marcgonzalez@google.com>
---
.../vc04_services/interface/vchiq_arm/vchiq_arm.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 319e7aa0e0e0..942b4768c88e 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -2081,7 +2081,7 @@ vchiq_dump(void *dump_context, const char *str, int len)
int copy_bytes;
if (context->offset > 0) {
- int skip_bytes = min(len, (int)context->offset);
+ int skip_bytes = min_t(int, len, context->offset);
str += skip_bytes;
len -= skip_bytes;
@@ -2089,7 +2089,7 @@ vchiq_dump(void *dump_context, const char *str, int len)
if (context->offset > 0)
return;
}
- copy_bytes = min(len, (int)(context->space - context->actual));
+ copy_bytes = min_t(int, len, context->space - context->actual);
if (copy_bytes == 0)
return;
if (copy_to_user(context->buf + context->actual, str,
@@ -2098,9 +2098,11 @@ vchiq_dump(void *dump_context, const char *str, int len)
context->actual += copy_bytes;
len -= copy_bytes;
- /* If tne terminating NUL is included in the length, then it
- ** marks the end of a line and should be replaced with a
- ** carriage return. */
+ /*
+ * If the terminating NUL is included in the length, then it
+ * marks the end of a line and should be replaced with a
+ * carriage return.
+ */
if ((len == 0) && (str[copy_bytes - 1] == '\0')) {
char cr = '\n';
--
2.20.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] staging: vchiq: Refactor indentation in vchiq_dump_* functions
2019-11-20 15:36 [PATCH 0/3] staging: vchiq: Fix vchiq_read return value in case of error Marcelo Diop-Gonzalez
2019-11-20 15:36 ` [PATCH 1/3] staging: vchiq_dump: Replace min with min_t Marcelo Diop-Gonzalez
@ 2019-11-20 15:36 ` Marcelo Diop-Gonzalez
2019-11-20 15:36 ` [PATCH 3/3] staging: vchiq: Have vchiq_dump_* functions return an error code Marcelo Diop-Gonzalez
` (2 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Marcelo Diop-Gonzalez @ 2019-11-20 15:36 UTC (permalink / raw)
To: gregkh, eric, wahrenst; +Cc: devel, linux-rpi-kernel
Doing this helps with readability, and makes
the logic easier to follow.
Signed-off-by: Marcelo Diop-Gonzalez <marcgonzalez@google.com>
---
.../interface/vchiq_arm/vchiq_arm.c | 104 +++++++++---------
1 file changed, 53 insertions(+), 51 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 942b4768c88e..8f9cfa083264 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -2076,40 +2076,40 @@ void
vchiq_dump(void *dump_context, const char *str, int len)
{
struct dump_context *context = (struct dump_context *)dump_context;
+ int copy_bytes;
- if (context->actual < context->space) {
- int copy_bytes;
+ if (context->actual >= context->space)
+ return;
- if (context->offset > 0) {
- int skip_bytes = min_t(int, len, context->offset);
+ if (context->offset > 0) {
+ int skip_bytes = min_t(int, len, context->offset);
- str += skip_bytes;
- len -= skip_bytes;
- context->offset -= skip_bytes;
- if (context->offset > 0)
- return;
- }
- copy_bytes = min_t(int, len, context->space - context->actual);
- if (copy_bytes == 0)
+ str += skip_bytes;
+ len -= skip_bytes;
+ context->offset -= skip_bytes;
+ if (context->offset > 0)
return;
- if (copy_to_user(context->buf + context->actual, str,
- copy_bytes))
+ }
+ copy_bytes = min_t(int, len, context->space - context->actual);
+ if (copy_bytes == 0)
+ return;
+ if (copy_to_user(context->buf + context->actual, str,
+ copy_bytes))
+ context->actual = -EFAULT;
+ context->actual += copy_bytes;
+ len -= copy_bytes;
+
+ /*
+ * If the terminating NUL is included in the length, then it
+ * marks the end of a line and should be replaced with a
+ * carriage return.
+ */
+ if ((len == 0) && (str[copy_bytes - 1] == '\0')) {
+ char cr = '\n';
+
+ if (copy_to_user(context->buf + context->actual - 1,
+ &cr, 1))
context->actual = -EFAULT;
- context->actual += copy_bytes;
- len -= copy_bytes;
-
- /*
- * If the terminating NUL is included in the length, then it
- * marks the end of a line and should be replaced with a
- * carriage return.
- */
- if ((len == 0) && (str[copy_bytes - 1] == '\0')) {
- char cr = '\n';
-
- if (copy_to_user(context->buf + context->actual - 1,
- &cr, 1))
- context->actual = -EFAULT;
- }
}
}
@@ -2134,34 +2134,36 @@ vchiq_dump_platform_instances(void *dump_context)
struct vchiq_service *service = state->services[i];
struct vchiq_instance *instance;
- if (service && (service->base.callback == service_callback)) {
- instance = service->instance;
- if (instance)
- instance->mark = 0;
- }
+ if (!service || service->base.callback != service_callback)
+ continue;
+
+ instance = service->instance;
+ if (instance)
+ instance->mark = 0;
}
for (i = 0; i < state->unused_service; i++) {
struct vchiq_service *service = state->services[i];
struct vchiq_instance *instance;
- if (service && (service->base.callback == service_callback)) {
- instance = service->instance;
- if (instance && !instance->mark) {
- len = snprintf(buf, sizeof(buf),
- "Instance %pK: pid %d,%s completions %d/%d",
- instance, instance->pid,
- instance->connected ? " connected, " :
- "",
- instance->completion_insert -
- instance->completion_remove,
- MAX_COMPLETIONS);
-
- vchiq_dump(dump_context, buf, len + 1);
-
- instance->mark = 1;
- }
- }
+ if (!service || service->base.callback != service_callback)
+ continue;
+
+ instance = service->instance;
+ if (!instance || instance->mark)
+ continue;
+
+ len = snprintf(buf, sizeof(buf),
+ "Instance %pK: pid %d,%s completions %d/%d",
+ instance, instance->pid,
+ instance->connected ? " connected, " :
+ "",
+ instance->completion_insert -
+ instance->completion_remove,
+ MAX_COMPLETIONS);
+
+ vchiq_dump(dump_context, buf, len + 1);
+ instance->mark = 1;
}
}
--
2.20.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] staging: vchiq: Have vchiq_dump_* functions return an error code
2019-11-20 15:36 [PATCH 0/3] staging: vchiq: Fix vchiq_read return value in case of error Marcelo Diop-Gonzalez
2019-11-20 15:36 ` [PATCH 1/3] staging: vchiq_dump: Replace min with min_t Marcelo Diop-Gonzalez
2019-11-20 15:36 ` [PATCH 2/3] staging: vchiq: Refactor indentation in vchiq_dump_* functions Marcelo Diop-Gonzalez
@ 2019-11-20 15:36 ` Marcelo Diop-Gonzalez
2019-11-20 18:50 ` Dan Carpenter
2019-11-20 20:20 ` [PATCH v2 0/4] staging: vchiq: Fix vchiq_read return value in case of error Marcelo Diop-Gonzalez
2019-11-21 10:42 ` [PATCH 0/3] " Stefan Wahren
4 siblings, 1 reply; 16+ messages in thread
From: Marcelo Diop-Gonzalez @ 2019-11-20 15:36 UTC (permalink / raw)
To: gregkh, eric, wahrenst; +Cc: devel, linux-rpi-kernel
These functions currently modify the struct dump_context passed
to them, and set context->actual to -EFAULT in case of error.
The issue is that this is never returned to the user (except
accidentally when things align so that that happens). So, have
these functions return 0 on success and the appropriate error
code otherwise, and return nonzero errors to the user.
Signed-off-by: Marcelo Diop-Gonzalez <marcgonzalez@google.com>
---
.../interface/vchiq_arm/vchiq_2835_arm.c | 5 +-
.../interface/vchiq_arm/vchiq_arm.c | 35 +++---
.../interface/vchiq_arm/vchiq_core.c | 104 +++++++++++++-----
.../interface/vchiq_arm/vchiq_core.h | 12 +-
4 files changed, 104 insertions(+), 52 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index e568e9e6eb95..ca30bfd52919 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -247,15 +247,14 @@ vchiq_complete_bulk(struct vchiq_bulk *bulk)
bulk->actual);
}
-void
-vchiq_dump_platform_state(void *dump_context)
+int vchiq_dump_platform_state(void *dump_context)
{
char buf[80];
int len;
len = snprintf(buf, sizeof(buf),
" Platform: 2835 (VC master)");
- vchiq_dump(dump_context, buf, len + 1);
+ return vchiq_dump(dump_context, buf, len + 1);
}
enum vchiq_status
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 8f9cfa083264..02148a24818a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -2072,14 +2072,13 @@ static int vchiq_release(struct inode *inode, struct file *file)
*
***************************************************************************/
-void
-vchiq_dump(void *dump_context, const char *str, int len)
+int vchiq_dump(void *dump_context, const char *str, int len)
{
struct dump_context *context = (struct dump_context *)dump_context;
int copy_bytes;
if (context->actual >= context->space)
- return;
+ return 0;
if (context->offset > 0) {
int skip_bytes = min_t(int, len, context->offset);
@@ -2088,14 +2087,14 @@ vchiq_dump(void *dump_context, const char *str, int len)
len -= skip_bytes;
context->offset -= skip_bytes;
if (context->offset > 0)
- return;
+ return 0;
}
copy_bytes = min_t(int, len, context->space - context->actual);
if (copy_bytes == 0)
- return;
+ return 0;
if (copy_to_user(context->buf + context->actual, str,
copy_bytes))
- context->actual = -EFAULT;
+ return -EFAULT;
context->actual += copy_bytes;
len -= copy_bytes;
@@ -2109,8 +2108,9 @@ vchiq_dump(void *dump_context, const char *str, int len)
if (copy_to_user(context->buf + context->actual - 1,
&cr, 1))
- context->actual = -EFAULT;
+ return -EFAULT;
}
+ return 0;
}
/****************************************************************************
@@ -2119,8 +2119,7 @@ vchiq_dump(void *dump_context, const char *str, int len)
*
***************************************************************************/
-void
-vchiq_dump_platform_instances(void *dump_context)
+int vchiq_dump_platform_instances(void *dump_context)
{
struct vchiq_state *state = vchiq_get_state();
char buf[80];
@@ -2145,6 +2144,7 @@ vchiq_dump_platform_instances(void *dump_context)
for (i = 0; i < state->unused_service; i++) {
struct vchiq_service *service = state->services[i];
struct vchiq_instance *instance;
+ int err;
if (!service || service->base.callback != service_callback)
continue;
@@ -2162,9 +2162,12 @@ vchiq_dump_platform_instances(void *dump_context)
instance->completion_remove,
MAX_COMPLETIONS);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
instance->mark = 1;
}
+ return 0;
}
/****************************************************************************
@@ -2173,9 +2176,8 @@ vchiq_dump_platform_instances(void *dump_context)
*
***************************************************************************/
-void
-vchiq_dump_platform_service_state(void *dump_context,
- struct vchiq_service *service)
+int vchiq_dump_platform_service_state(void *dump_context,
+ struct vchiq_service *service)
{
struct user_service *user_service =
(struct user_service *)service->base.userdata;
@@ -2196,7 +2198,7 @@ vchiq_dump_platform_service_state(void *dump_context,
" (dequeue pending)");
}
- vchiq_dump(dump_context, buf, len + 1);
+ return vchiq_dump(dump_context, buf, len + 1);
}
/****************************************************************************
@@ -2210,13 +2212,16 @@ vchiq_read(struct file *file, char __user *buf,
size_t count, loff_t *ppos)
{
struct dump_context context;
+ int err;
context.buf = buf;
context.actual = 0;
context.space = count;
context.offset = *ppos;
- vchiq_dump_state(&context, &g_state);
+ err = vchiq_dump_state(&context, &g_state);
+ if (err)
+ return err;
*ppos += context.actual;
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index b9d94f6b9bef..76351078affb 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3298,7 +3298,7 @@ vchiq_set_service_option(unsigned int handle,
return status;
}
-static void
+static int
vchiq_dump_shared_state(void *dump_context, struct vchiq_state *state,
struct vchiq_shared_state *shared, const char *label)
{
@@ -3318,16 +3318,21 @@ vchiq_dump_shared_state(void *dump_context, struct vchiq_state *state,
int i;
char buf[80];
int len;
+ int err;
len = scnprintf(buf, sizeof(buf),
" %s: slots %d-%d tx_pos=%x recycle=%x",
label, shared->slot_first, shared->slot_last,
shared->tx_pos, shared->slot_queue_recycle);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
len = scnprintf(buf, sizeof(buf),
" Slots claimed:");
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
for (i = shared->slot_first; i <= shared->slot_last; i++) {
struct vchiq_slot_info slot_info =
@@ -3336,27 +3341,34 @@ vchiq_dump_shared_state(void *dump_context, struct vchiq_state *state,
len = scnprintf(buf, sizeof(buf),
" %d: %d/%d", i, slot_info.use_count,
slot_info.release_count);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
}
}
for (i = 1; i < shared->debug[DEBUG_ENTRIES]; i++) {
len = scnprintf(buf, sizeof(buf), " DEBUG: %s = %d(%x)",
debug_names[i], shared->debug[i], shared->debug[i]);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
}
+ return 0;
}
-void
-vchiq_dump_state(void *dump_context, struct vchiq_state *state)
+int vchiq_dump_state(void *dump_context, struct vchiq_state *state)
{
char buf[80];
int len;
int i;
+ int err;
len = scnprintf(buf, sizeof(buf), "State %d: %s", state->id,
conn_state_names[state->conn_state]);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
len = scnprintf(buf, sizeof(buf),
" tx_pos=%x(@%pK), rx_pos=%x(@%pK)",
@@ -3364,12 +3376,16 @@ vchiq_dump_state(void *dump_context, struct vchiq_state *state)
state->tx_data + (state->local_tx_pos & VCHIQ_SLOT_MASK),
state->rx_pos,
state->rx_data + (state->rx_pos & VCHIQ_SLOT_MASK));
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
len = scnprintf(buf, sizeof(buf),
" Version: %d (min %d)",
VCHIQ_VERSION, VCHIQ_VERSION_MIN);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
if (VCHIQ_ENABLE_STATS) {
len = scnprintf(buf, sizeof(buf),
@@ -3377,7 +3393,9 @@ vchiq_dump_state(void *dump_context, struct vchiq_state *state)
"error_count=%d",
state->stats.ctrl_tx_count, state->stats.ctrl_rx_count,
state->stats.error_count);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
}
len = scnprintf(buf, sizeof(buf),
@@ -3388,30 +3406,49 @@ vchiq_dump_state(void *dump_context, struct vchiq_state *state)
state->data_quota - state->data_use_count,
state->local->slot_queue_recycle - state->slot_queue_available,
state->stats.slot_stalls, state->stats.data_stalls);
- vchiq_dump(dump_context, buf, len + 1);
-
- vchiq_dump_platform_state(dump_context);
-
- vchiq_dump_shared_state(dump_context, state, state->local, "Local");
- vchiq_dump_shared_state(dump_context, state, state->remote, "Remote");
-
- vchiq_dump_platform_instances(dump_context);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
+
+ err = vchiq_dump_platform_state(dump_context);
+ if (err)
+ return err;
+
+ err = vchiq_dump_shared_state(dump_context,
+ state,
+ state->local,
+ "Local");
+ if (err)
+ return err;
+ err = vchiq_dump_shared_state(dump_context,
+ state,
+ state->remote,
+ "Remote");
+ if (err)
+ return err;
+
+ err = vchiq_dump_platform_instances(dump_context);
+ if (err)
+ return err;
for (i = 0; i < state->unused_service; i++) {
struct vchiq_service *service = find_service_by_port(state, i);
if (service) {
- vchiq_dump_service_state(dump_context, service);
+ err = vchiq_dump_service_state(dump_context, service);
unlock_service(service);
+ if (err)
+ return err;
}
}
+ return 0;
}
-void
-vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
+int vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
{
char buf[80];
int len;
+ int err;
len = scnprintf(buf, sizeof(buf), "Service %u: %s (ref %u)",
service->localport, srvstate_names[service->srvstate],
@@ -3444,7 +3481,9 @@ vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
service_quota->slot_use_count,
service_quota->slot_quota);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
tx_pending = service->bulk_tx.local_insert -
service->bulk_tx.remote_insert;
@@ -3463,7 +3502,9 @@ vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
BULK_INDEX(service->bulk_rx.remove)].size : 0);
if (VCHIQ_ENABLE_STATS) {
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
len = scnprintf(buf, sizeof(buf),
" Ctrl: tx_count=%d, tx_bytes=%llu, "
@@ -3472,7 +3513,9 @@ vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
service->stats.ctrl_tx_bytes,
service->stats.ctrl_rx_count,
service->stats.ctrl_rx_bytes);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
len = scnprintf(buf, sizeof(buf),
" Bulk: tx_count=%d, tx_bytes=%llu, "
@@ -3481,7 +3524,9 @@ vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
service->stats.bulk_tx_bytes,
service->stats.bulk_rx_count,
service->stats.bulk_rx_bytes);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
len = scnprintf(buf, sizeof(buf),
" %d quota stalls, %d slot stalls, "
@@ -3494,10 +3539,13 @@ vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
}
}
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
if (service->srvstate != VCHIQ_SRVSTATE_FREE)
- vchiq_dump_platform_service_state(dump_context, service);
+ err = vchiq_dump_platform_service_state(dump_context, service);
+ return err;
}
void
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index 419bcdd165b4..c31f953a9986 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -526,10 +526,10 @@ vchiq_bulk_transfer(unsigned int handle, void *offset, int size,
void *userdata, enum vchiq_bulk_mode mode,
enum vchiq_bulk_dir dir);
-extern void
+extern int
vchiq_dump_state(void *dump_context, struct vchiq_state *state);
-extern void
+extern int
vchiq_dump_service_state(void *dump_context, struct vchiq_service *service);
extern void
@@ -602,16 +602,16 @@ vchiq_platform_resume(struct vchiq_state *state);
extern void
vchiq_platform_resumed(struct vchiq_state *state);
-extern void
+extern int
vchiq_dump(void *dump_context, const char *str, int len);
-extern void
+extern int
vchiq_dump_platform_state(void *dump_context);
-extern void
+extern int
vchiq_dump_platform_instances(void *dump_context);
-extern void
+extern int
vchiq_dump_platform_service_state(void *dump_context,
struct vchiq_service *service);
--
2.20.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] staging: vchiq_dump: Replace min with min_t
2019-11-20 15:36 ` [PATCH 1/3] staging: vchiq_dump: Replace min with min_t Marcelo Diop-Gonzalez
@ 2019-11-20 15:46 ` Greg KH
0 siblings, 0 replies; 16+ messages in thread
From: Greg KH @ 2019-11-20 15:46 UTC (permalink / raw)
To: Marcelo Diop-Gonzalez; +Cc: devel, eric, wahrenst, linux-rpi-kernel
On Wed, Nov 20, 2019 at 10:36:46AM -0500, Marcelo Diop-Gonzalez wrote:
> Replacing this and fixing the block comment format in this
> function fixes checkpatch warnings.
That is two different things. Which means you need two different
patches here.
Please fix up.
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] staging: vchiq: Have vchiq_dump_* functions return an error code
2019-11-20 15:36 ` [PATCH 3/3] staging: vchiq: Have vchiq_dump_* functions return an error code Marcelo Diop-Gonzalez
@ 2019-11-20 18:50 ` Dan Carpenter
2019-11-21 21:25 ` Marcelo Diop-Gonzalez
0 siblings, 1 reply; 16+ messages in thread
From: Dan Carpenter @ 2019-11-20 18:50 UTC (permalink / raw)
To: Marcelo Diop-Gonzalez; +Cc: devel, gregkh, linux-rpi-kernel, wahrenst, eric
Thanks for the patch. Looks good.
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
On Wed, Nov 20, 2019 at 10:36:48AM -0500, Marcelo Diop-Gonzalez wrote:
> copy_bytes = min_t(int, len, context->space - context->actual);
> if (copy_bytes == 0)
> - return;
> + return 0;
> if (copy_to_user(context->buf + context->actual, str,
> copy_bytes))
> - context->actual = -EFAULT;
^^^^^^^^^^^^^^^^^^^^^^^^^
> + return -EFAULT;
> context->actual += copy_bytes;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
In the original code we saved the error code to context->actual, then
we added some bytes
> len -= copy_bytes;
>
> @@ -2109,8 +2108,9 @@ vchiq_dump(void *dump_context, const char *str, int len)
>
> if (copy_to_user(context->buf + context->actual - 1,
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
and then we tried to use the result to corrupt the user space program.
So this patch is a nice bug fix.
> &cr, 1))
> - context->actual = -EFAULT;
> + return -EFAULT;
> }
> + return 0;
> }
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 0/4] staging: vchiq: Fix vchiq_read return value in case of error
2019-11-20 15:36 [PATCH 0/3] staging: vchiq: Fix vchiq_read return value in case of error Marcelo Diop-Gonzalez
` (2 preceding siblings ...)
2019-11-20 15:36 ` [PATCH 3/3] staging: vchiq: Have vchiq_dump_* functions return an error code Marcelo Diop-Gonzalez
@ 2019-11-20 20:20 ` Marcelo Diop-Gonzalez
2019-11-20 20:20 ` [PATCH v2 1/4] staging: vchiq: Fix block comment format in vchiq_dump() Marcelo Diop-Gonzalez
` (4 more replies)
2019-11-21 10:42 ` [PATCH 0/3] " Stefan Wahren
4 siblings, 5 replies; 16+ messages in thread
From: Marcelo Diop-Gonzalez @ 2019-11-20 20:20 UTC (permalink / raw)
To: gregkh, eric, wahrenst; +Cc: devel, linux-rpi-kernel
This is a proposed fix of an issue regarding the handling of positive
return values from copy_to_user() in vchiq_read(). It looks like the
struct dump_context passed to the vchiq_dump() function keeps track
of the number of bytes written in the context->actual field. When
copy_to_user() returns a positive value, this is set to -EFAULT. The
problem is that this is never returned to the user, and instead the
function continues, adding the number of bytes that should have
been copied to context->actual.
Running the following program:
#include <stdio.h>
#include <stdlib.h>
#include <fcntl.h>
#include <unistd.h>
#include <errno.h>
int main(int argc, char **argv) {
int fd = open("/dev/vchiq", 0);
if (fd < 0)
exit(-1);
sbrk(getpagesize());
char *bad_buf = sbrk(0)-100;
int x = read(fd, bad_buf, 2000);
printf("%d %d\n", x, errno);
puts(bad_buf);
}
gives this output:
-1 1
State 0: CONNECTED
tx_po
Remote: slots 2-32 tx_pos=578 recycle=1f
Slots claimed:
Note the mangled output and incorrect errno value. Messing with the
constants in that toy program changes the results. Sometimes read()
returns with no error, sometimes it returns with a wrong error code,
sometimes with the right one. But it seems that it only ever returns an
error at all accidentally, due to the fact that the comparison between
context->actual and context->space in vchiq_dump() is unsigned, so that
that function won't do anything else if it ever sets context->actual
to a negative value.
After this patchset, the above program prints this:
-1 14
State 0: CONNECTED
tx_pos=b4a218(@165de6b4), rx_pos=ae0668(@f02b54f4)
Version: 8 (min 3)
Stats
Help with testing would be appreciated. So far I've basically just
diffed the output of 'cat /dev/vchiq', run the program above with
a few different values, and run vchiq_test a few times.
These were applied to the staging-next branch of the tree
at git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
v2:
separate first patch into different logical changes.
Marcelo Diop-Gonzalez (4):
staging: vchiq: Fix block comment format in vchiq_dump()
staging: vchiq_dump: Replace min with min_t
staging: vchiq: Refactor indentation in vchiq_dump_* functions
staging: vchiq: Have vchiq_dump_* functions return an error code
.../interface/vchiq_arm/vchiq_2835_arm.c | 5 +-
.../interface/vchiq_arm/vchiq_arm.c | 129 ++++++++++--------
.../interface/vchiq_arm/vchiq_core.c | 104 ++++++++++----
.../interface/vchiq_arm/vchiq_core.h | 12 +-
4 files changed, 153 insertions(+), 97 deletions(-)
--
2.24.0.432.g9d3f5f5b63-goog
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/4] staging: vchiq: Fix block comment format in vchiq_dump()
2019-11-20 20:20 ` [PATCH v2 0/4] staging: vchiq: Fix vchiq_read return value in case of error Marcelo Diop-Gonzalez
@ 2019-11-20 20:20 ` Marcelo Diop-Gonzalez
2019-11-20 20:21 ` [PATCH v2 2/4] staging: vchiq_dump: Replace min with min_t Marcelo Diop-Gonzalez
` (3 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Marcelo Diop-Gonzalez @ 2019-11-20 20:20 UTC (permalink / raw)
To: gregkh, eric, wahrenst; +Cc: devel, linux-rpi-kernel
This fixes a checkpatch warning.
Signed-off-by: Marcelo Diop-Gonzalez <marcgonzalez@google.com>
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 319e7aa0e0e0..6328abcaeeeb 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -2098,9 +2098,11 @@ vchiq_dump(void *dump_context, const char *str, int len)
context->actual += copy_bytes;
len -= copy_bytes;
- /* If tne terminating NUL is included in the length, then it
- ** marks the end of a line and should be replaced with a
- ** carriage return. */
+ /*
+ * If the terminating NUL is included in the length, then it
+ * marks the end of a line and should be replaced with a
+ * carriage return.
+ */
if ((len == 0) && (str[copy_bytes - 1] == '\0')) {
char cr = '\n';
--
2.24.0.432.g9d3f5f5b63-goog
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/4] staging: vchiq_dump: Replace min with min_t
2019-11-20 20:20 ` [PATCH v2 0/4] staging: vchiq: Fix vchiq_read return value in case of error Marcelo Diop-Gonzalez
2019-11-20 20:20 ` [PATCH v2 1/4] staging: vchiq: Fix block comment format in vchiq_dump() Marcelo Diop-Gonzalez
@ 2019-11-20 20:21 ` Marcelo Diop-Gonzalez
2019-11-20 20:21 ` [PATCH v2 3/4] staging: vchiq: Refactor indentation in vchiq_dump_* functions Marcelo Diop-Gonzalez
` (2 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Marcelo Diop-Gonzalez @ 2019-11-20 20:21 UTC (permalink / raw)
To: gregkh, eric, wahrenst; +Cc: devel, linux-rpi-kernel
Replacing this fixes checkpatch warnings.
Signed-off-by: Marcelo Diop-Gonzalez <marcgonzalez@google.com>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 6328abcaeeeb..942b4768c88e 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -2081,7 +2081,7 @@ vchiq_dump(void *dump_context, const char *str, int len)
int copy_bytes;
if (context->offset > 0) {
- int skip_bytes = min(len, (int)context->offset);
+ int skip_bytes = min_t(int, len, context->offset);
str += skip_bytes;
len -= skip_bytes;
@@ -2089,7 +2089,7 @@ vchiq_dump(void *dump_context, const char *str, int len)
if (context->offset > 0)
return;
}
- copy_bytes = min(len, (int)(context->space - context->actual));
+ copy_bytes = min_t(int, len, context->space - context->actual);
if (copy_bytes == 0)
return;
if (copy_to_user(context->buf + context->actual, str,
--
2.24.0.432.g9d3f5f5b63-goog
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 3/4] staging: vchiq: Refactor indentation in vchiq_dump_* functions
2019-11-20 20:20 ` [PATCH v2 0/4] staging: vchiq: Fix vchiq_read return value in case of error Marcelo Diop-Gonzalez
2019-11-20 20:20 ` [PATCH v2 1/4] staging: vchiq: Fix block comment format in vchiq_dump() Marcelo Diop-Gonzalez
2019-11-20 20:21 ` [PATCH v2 2/4] staging: vchiq_dump: Replace min with min_t Marcelo Diop-Gonzalez
@ 2019-11-20 20:21 ` Marcelo Diop-Gonzalez
2019-11-20 20:21 ` [PATCH v2 4/4] staging: vchiq: Have vchiq_dump_* functions return an error code Marcelo Diop-Gonzalez
2019-11-21 19:38 ` [PATCH v2 0/4] staging: vchiq: Fix vchiq_read return value in case of error Nicolas Saenz Julienne
4 siblings, 0 replies; 16+ messages in thread
From: Marcelo Diop-Gonzalez @ 2019-11-20 20:21 UTC (permalink / raw)
To: gregkh, eric, wahrenst; +Cc: devel, linux-rpi-kernel
Doing this helps with readability, and makes
the logic easier to follow.
Signed-off-by: Marcelo Diop-Gonzalez <marcgonzalez@google.com>
---
.../interface/vchiq_arm/vchiq_arm.c | 104 +++++++++---------
1 file changed, 53 insertions(+), 51 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 942b4768c88e..8f9cfa083264 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -2076,40 +2076,40 @@ void
vchiq_dump(void *dump_context, const char *str, int len)
{
struct dump_context *context = (struct dump_context *)dump_context;
+ int copy_bytes;
- if (context->actual < context->space) {
- int copy_bytes;
+ if (context->actual >= context->space)
+ return;
- if (context->offset > 0) {
- int skip_bytes = min_t(int, len, context->offset);
+ if (context->offset > 0) {
+ int skip_bytes = min_t(int, len, context->offset);
- str += skip_bytes;
- len -= skip_bytes;
- context->offset -= skip_bytes;
- if (context->offset > 0)
- return;
- }
- copy_bytes = min_t(int, len, context->space - context->actual);
- if (copy_bytes == 0)
+ str += skip_bytes;
+ len -= skip_bytes;
+ context->offset -= skip_bytes;
+ if (context->offset > 0)
return;
- if (copy_to_user(context->buf + context->actual, str,
- copy_bytes))
+ }
+ copy_bytes = min_t(int, len, context->space - context->actual);
+ if (copy_bytes == 0)
+ return;
+ if (copy_to_user(context->buf + context->actual, str,
+ copy_bytes))
+ context->actual = -EFAULT;
+ context->actual += copy_bytes;
+ len -= copy_bytes;
+
+ /*
+ * If the terminating NUL is included in the length, then it
+ * marks the end of a line and should be replaced with a
+ * carriage return.
+ */
+ if ((len == 0) && (str[copy_bytes - 1] == '\0')) {
+ char cr = '\n';
+
+ if (copy_to_user(context->buf + context->actual - 1,
+ &cr, 1))
context->actual = -EFAULT;
- context->actual += copy_bytes;
- len -= copy_bytes;
-
- /*
- * If the terminating NUL is included in the length, then it
- * marks the end of a line and should be replaced with a
- * carriage return.
- */
- if ((len == 0) && (str[copy_bytes - 1] == '\0')) {
- char cr = '\n';
-
- if (copy_to_user(context->buf + context->actual - 1,
- &cr, 1))
- context->actual = -EFAULT;
- }
}
}
@@ -2134,34 +2134,36 @@ vchiq_dump_platform_instances(void *dump_context)
struct vchiq_service *service = state->services[i];
struct vchiq_instance *instance;
- if (service && (service->base.callback == service_callback)) {
- instance = service->instance;
- if (instance)
- instance->mark = 0;
- }
+ if (!service || service->base.callback != service_callback)
+ continue;
+
+ instance = service->instance;
+ if (instance)
+ instance->mark = 0;
}
for (i = 0; i < state->unused_service; i++) {
struct vchiq_service *service = state->services[i];
struct vchiq_instance *instance;
- if (service && (service->base.callback == service_callback)) {
- instance = service->instance;
- if (instance && !instance->mark) {
- len = snprintf(buf, sizeof(buf),
- "Instance %pK: pid %d,%s completions %d/%d",
- instance, instance->pid,
- instance->connected ? " connected, " :
- "",
- instance->completion_insert -
- instance->completion_remove,
- MAX_COMPLETIONS);
-
- vchiq_dump(dump_context, buf, len + 1);
-
- instance->mark = 1;
- }
- }
+ if (!service || service->base.callback != service_callback)
+ continue;
+
+ instance = service->instance;
+ if (!instance || instance->mark)
+ continue;
+
+ len = snprintf(buf, sizeof(buf),
+ "Instance %pK: pid %d,%s completions %d/%d",
+ instance, instance->pid,
+ instance->connected ? " connected, " :
+ "",
+ instance->completion_insert -
+ instance->completion_remove,
+ MAX_COMPLETIONS);
+
+ vchiq_dump(dump_context, buf, len + 1);
+ instance->mark = 1;
}
}
--
2.24.0.432.g9d3f5f5b63-goog
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 4/4] staging: vchiq: Have vchiq_dump_* functions return an error code
2019-11-20 20:20 ` [PATCH v2 0/4] staging: vchiq: Fix vchiq_read return value in case of error Marcelo Diop-Gonzalez
` (2 preceding siblings ...)
2019-11-20 20:21 ` [PATCH v2 3/4] staging: vchiq: Refactor indentation in vchiq_dump_* functions Marcelo Diop-Gonzalez
@ 2019-11-20 20:21 ` Marcelo Diop-Gonzalez
2019-11-21 19:38 ` [PATCH v2 0/4] staging: vchiq: Fix vchiq_read return value in case of error Nicolas Saenz Julienne
4 siblings, 0 replies; 16+ messages in thread
From: Marcelo Diop-Gonzalez @ 2019-11-20 20:21 UTC (permalink / raw)
To: gregkh, eric, wahrenst; +Cc: devel, linux-rpi-kernel, Dan Carpenter
These functions currently modify the struct dump_context passed
to them, and set context->actual to -EFAULT in case of error.
The issue is that this is never returned to the user (except
accidentally when things align so that that happens). So, have
these functions return 0 on success and the appropriate error
code otherwise, and return nonzero errors to the user.
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Marcelo Diop-Gonzalez <marcgonzalez@google.com>
---
.../interface/vchiq_arm/vchiq_2835_arm.c | 5 +-
.../interface/vchiq_arm/vchiq_arm.c | 35 +++---
.../interface/vchiq_arm/vchiq_core.c | 104 +++++++++++++-----
.../interface/vchiq_arm/vchiq_core.h | 12 +-
4 files changed, 104 insertions(+), 52 deletions(-)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
index e568e9e6eb95..ca30bfd52919 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
@@ -247,15 +247,14 @@ vchiq_complete_bulk(struct vchiq_bulk *bulk)
bulk->actual);
}
-void
-vchiq_dump_platform_state(void *dump_context)
+int vchiq_dump_platform_state(void *dump_context)
{
char buf[80];
int len;
len = snprintf(buf, sizeof(buf),
" Platform: 2835 (VC master)");
- vchiq_dump(dump_context, buf, len + 1);
+ return vchiq_dump(dump_context, buf, len + 1);
}
enum vchiq_status
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 8f9cfa083264..02148a24818a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -2072,14 +2072,13 @@ static int vchiq_release(struct inode *inode, struct file *file)
*
***************************************************************************/
-void
-vchiq_dump(void *dump_context, const char *str, int len)
+int vchiq_dump(void *dump_context, const char *str, int len)
{
struct dump_context *context = (struct dump_context *)dump_context;
int copy_bytes;
if (context->actual >= context->space)
- return;
+ return 0;
if (context->offset > 0) {
int skip_bytes = min_t(int, len, context->offset);
@@ -2088,14 +2087,14 @@ vchiq_dump(void *dump_context, const char *str, int len)
len -= skip_bytes;
context->offset -= skip_bytes;
if (context->offset > 0)
- return;
+ return 0;
}
copy_bytes = min_t(int, len, context->space - context->actual);
if (copy_bytes == 0)
- return;
+ return 0;
if (copy_to_user(context->buf + context->actual, str,
copy_bytes))
- context->actual = -EFAULT;
+ return -EFAULT;
context->actual += copy_bytes;
len -= copy_bytes;
@@ -2109,8 +2108,9 @@ vchiq_dump(void *dump_context, const char *str, int len)
if (copy_to_user(context->buf + context->actual - 1,
&cr, 1))
- context->actual = -EFAULT;
+ return -EFAULT;
}
+ return 0;
}
/****************************************************************************
@@ -2119,8 +2119,7 @@ vchiq_dump(void *dump_context, const char *str, int len)
*
***************************************************************************/
-void
-vchiq_dump_platform_instances(void *dump_context)
+int vchiq_dump_platform_instances(void *dump_context)
{
struct vchiq_state *state = vchiq_get_state();
char buf[80];
@@ -2145,6 +2144,7 @@ vchiq_dump_platform_instances(void *dump_context)
for (i = 0; i < state->unused_service; i++) {
struct vchiq_service *service = state->services[i];
struct vchiq_instance *instance;
+ int err;
if (!service || service->base.callback != service_callback)
continue;
@@ -2162,9 +2162,12 @@ vchiq_dump_platform_instances(void *dump_context)
instance->completion_remove,
MAX_COMPLETIONS);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
instance->mark = 1;
}
+ return 0;
}
/****************************************************************************
@@ -2173,9 +2176,8 @@ vchiq_dump_platform_instances(void *dump_context)
*
***************************************************************************/
-void
-vchiq_dump_platform_service_state(void *dump_context,
- struct vchiq_service *service)
+int vchiq_dump_platform_service_state(void *dump_context,
+ struct vchiq_service *service)
{
struct user_service *user_service =
(struct user_service *)service->base.userdata;
@@ -2196,7 +2198,7 @@ vchiq_dump_platform_service_state(void *dump_context,
" (dequeue pending)");
}
- vchiq_dump(dump_context, buf, len + 1);
+ return vchiq_dump(dump_context, buf, len + 1);
}
/****************************************************************************
@@ -2210,13 +2212,16 @@ vchiq_read(struct file *file, char __user *buf,
size_t count, loff_t *ppos)
{
struct dump_context context;
+ int err;
context.buf = buf;
context.actual = 0;
context.space = count;
context.offset = *ppos;
- vchiq_dump_state(&context, &g_state);
+ err = vchiq_dump_state(&context, &g_state);
+ if (err)
+ return err;
*ppos += context.actual;
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index b9d94f6b9bef..76351078affb 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -3298,7 +3298,7 @@ vchiq_set_service_option(unsigned int handle,
return status;
}
-static void
+static int
vchiq_dump_shared_state(void *dump_context, struct vchiq_state *state,
struct vchiq_shared_state *shared, const char *label)
{
@@ -3318,16 +3318,21 @@ vchiq_dump_shared_state(void *dump_context, struct vchiq_state *state,
int i;
char buf[80];
int len;
+ int err;
len = scnprintf(buf, sizeof(buf),
" %s: slots %d-%d tx_pos=%x recycle=%x",
label, shared->slot_first, shared->slot_last,
shared->tx_pos, shared->slot_queue_recycle);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
len = scnprintf(buf, sizeof(buf),
" Slots claimed:");
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
for (i = shared->slot_first; i <= shared->slot_last; i++) {
struct vchiq_slot_info slot_info =
@@ -3336,27 +3341,34 @@ vchiq_dump_shared_state(void *dump_context, struct vchiq_state *state,
len = scnprintf(buf, sizeof(buf),
" %d: %d/%d", i, slot_info.use_count,
slot_info.release_count);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
}
}
for (i = 1; i < shared->debug[DEBUG_ENTRIES]; i++) {
len = scnprintf(buf, sizeof(buf), " DEBUG: %s = %d(%x)",
debug_names[i], shared->debug[i], shared->debug[i]);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
}
+ return 0;
}
-void
-vchiq_dump_state(void *dump_context, struct vchiq_state *state)
+int vchiq_dump_state(void *dump_context, struct vchiq_state *state)
{
char buf[80];
int len;
int i;
+ int err;
len = scnprintf(buf, sizeof(buf), "State %d: %s", state->id,
conn_state_names[state->conn_state]);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
len = scnprintf(buf, sizeof(buf),
" tx_pos=%x(@%pK), rx_pos=%x(@%pK)",
@@ -3364,12 +3376,16 @@ vchiq_dump_state(void *dump_context, struct vchiq_state *state)
state->tx_data + (state->local_tx_pos & VCHIQ_SLOT_MASK),
state->rx_pos,
state->rx_data + (state->rx_pos & VCHIQ_SLOT_MASK));
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
len = scnprintf(buf, sizeof(buf),
" Version: %d (min %d)",
VCHIQ_VERSION, VCHIQ_VERSION_MIN);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
if (VCHIQ_ENABLE_STATS) {
len = scnprintf(buf, sizeof(buf),
@@ -3377,7 +3393,9 @@ vchiq_dump_state(void *dump_context, struct vchiq_state *state)
"error_count=%d",
state->stats.ctrl_tx_count, state->stats.ctrl_rx_count,
state->stats.error_count);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
}
len = scnprintf(buf, sizeof(buf),
@@ -3388,30 +3406,49 @@ vchiq_dump_state(void *dump_context, struct vchiq_state *state)
state->data_quota - state->data_use_count,
state->local->slot_queue_recycle - state->slot_queue_available,
state->stats.slot_stalls, state->stats.data_stalls);
- vchiq_dump(dump_context, buf, len + 1);
-
- vchiq_dump_platform_state(dump_context);
-
- vchiq_dump_shared_state(dump_context, state, state->local, "Local");
- vchiq_dump_shared_state(dump_context, state, state->remote, "Remote");
-
- vchiq_dump_platform_instances(dump_context);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
+
+ err = vchiq_dump_platform_state(dump_context);
+ if (err)
+ return err;
+
+ err = vchiq_dump_shared_state(dump_context,
+ state,
+ state->local,
+ "Local");
+ if (err)
+ return err;
+ err = vchiq_dump_shared_state(dump_context,
+ state,
+ state->remote,
+ "Remote");
+ if (err)
+ return err;
+
+ err = vchiq_dump_platform_instances(dump_context);
+ if (err)
+ return err;
for (i = 0; i < state->unused_service; i++) {
struct vchiq_service *service = find_service_by_port(state, i);
if (service) {
- vchiq_dump_service_state(dump_context, service);
+ err = vchiq_dump_service_state(dump_context, service);
unlock_service(service);
+ if (err)
+ return err;
}
}
+ return 0;
}
-void
-vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
+int vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
{
char buf[80];
int len;
+ int err;
len = scnprintf(buf, sizeof(buf), "Service %u: %s (ref %u)",
service->localport, srvstate_names[service->srvstate],
@@ -3444,7 +3481,9 @@ vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
service_quota->slot_use_count,
service_quota->slot_quota);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
tx_pending = service->bulk_tx.local_insert -
service->bulk_tx.remote_insert;
@@ -3463,7 +3502,9 @@ vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
BULK_INDEX(service->bulk_rx.remove)].size : 0);
if (VCHIQ_ENABLE_STATS) {
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
len = scnprintf(buf, sizeof(buf),
" Ctrl: tx_count=%d, tx_bytes=%llu, "
@@ -3472,7 +3513,9 @@ vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
service->stats.ctrl_tx_bytes,
service->stats.ctrl_rx_count,
service->stats.ctrl_rx_bytes);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
len = scnprintf(buf, sizeof(buf),
" Bulk: tx_count=%d, tx_bytes=%llu, "
@@ -3481,7 +3524,9 @@ vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
service->stats.bulk_tx_bytes,
service->stats.bulk_rx_count,
service->stats.bulk_rx_bytes);
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
len = scnprintf(buf, sizeof(buf),
" %d quota stalls, %d slot stalls, "
@@ -3494,10 +3539,13 @@ vchiq_dump_service_state(void *dump_context, struct vchiq_service *service)
}
}
- vchiq_dump(dump_context, buf, len + 1);
+ err = vchiq_dump(dump_context, buf, len + 1);
+ if (err)
+ return err;
if (service->srvstate != VCHIQ_SRVSTATE_FREE)
- vchiq_dump_platform_service_state(dump_context, service);
+ err = vchiq_dump_platform_service_state(dump_context, service);
+ return err;
}
void
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index 419bcdd165b4..c31f953a9986 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -526,10 +526,10 @@ vchiq_bulk_transfer(unsigned int handle, void *offset, int size,
void *userdata, enum vchiq_bulk_mode mode,
enum vchiq_bulk_dir dir);
-extern void
+extern int
vchiq_dump_state(void *dump_context, struct vchiq_state *state);
-extern void
+extern int
vchiq_dump_service_state(void *dump_context, struct vchiq_service *service);
extern void
@@ -602,16 +602,16 @@ vchiq_platform_resume(struct vchiq_state *state);
extern void
vchiq_platform_resumed(struct vchiq_state *state);
-extern void
+extern int
vchiq_dump(void *dump_context, const char *str, int len);
-extern void
+extern int
vchiq_dump_platform_state(void *dump_context);
-extern void
+extern int
vchiq_dump_platform_instances(void *dump_context);
-extern void
+extern int
vchiq_dump_platform_service_state(void *dump_context,
struct vchiq_service *service);
--
2.24.0.432.g9d3f5f5b63-goog
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] staging: vchiq: Fix vchiq_read return value in case of error
2019-11-20 15:36 [PATCH 0/3] staging: vchiq: Fix vchiq_read return value in case of error Marcelo Diop-Gonzalez
` (3 preceding siblings ...)
2019-11-20 20:20 ` [PATCH v2 0/4] staging: vchiq: Fix vchiq_read return value in case of error Marcelo Diop-Gonzalez
@ 2019-11-21 10:42 ` Stefan Wahren
2019-11-21 19:41 ` Nicolas Saenz Julienne
4 siblings, 1 reply; 16+ messages in thread
From: Stefan Wahren @ 2019-11-21 10:42 UTC (permalink / raw)
To: Marcelo Diop-Gonzalez, gregkh, eric, wahrenst
Cc: devel, Phil Elwell, linux-rpi-kernel
Hi Marcelo,
[add Nicolas and Phil]
Am 20.11.19 um 16:36 schrieb Marcelo Diop-Gonzalez:
> This is a proposed fix of an issue regarding the handling of positive
> return values from copy_to_user() in vchiq_read(). It looks like the
> struct dump_context passed to the vchiq_dump() function keeps track
> of the number of bytes written in the context->actual field. When
> copy_to_user() returns a positive value, this is set to -EFAULT. The
> problem is that this is never returned to the user, and instead the function
> continues, adding the number of bytes that should have been copied
> to context->actual.
>
> Running the following program:
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <fcntl.h>
> #include <unistd.h>
> #include <errno.h>
>
> int main(int argc, char **argv) {
> int fd = open("/dev/vchiq", 0);
> if (fd < 0)
> exit(-1);
> sbrk(getpagesize());
> char *bad_buf = sbrk(0)-100;
> int x = read(fd, bad_buf, 2000);
> printf("%d %d\n", x, errno);
> puts(bad_buf);
> }
>
> gives this output:
>
> -1 1
> State 0: CONNECTED
> tx_po
>
>
> Remote: slots 2-32 tx_pos=578 recycle=1f
> Slots claimed:
>
> Note the mangled output and incorrect errno value. Messing with the
> constants in that toy program changes the results. Sometimes read()
> returns with no error, sometimes it returns with a wrong error code,
> sometimes with the right one. But it seems that it only ever returns an
> error at all accidentally, due to the fact that the comparison between
> context->actual and context->space in vchiq_dump() is unsigned, so that
> that function won't do anything else if it ever sets context->actual
> to a negative value.
>
> After this patchset, the above program prints this:
>
> -1 14
> State 0: CONNECTED
> tx_pos=b4a218(@165de6b4), rx_pos=ae0668(@f02b54f4)
> Version: 8 (min 3)
> Stats
>
> Help with testing would be appreciated. So far I've basically just
> diffed the output of 'cat /dev/vchiq', run the program above with
> a few different values, and run vchiq_test a few times.
i consider this as sufficient, but i'm not the VCHIQ expert.
Thanks
Stefan
>
> These were applied to the staging-next branch of the tree
> at git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
>
> Marcelo Diop-Gonzalez (3):
> staging: vchiq_dump: Replace min with min_t
> staging: vchiq: Refactor indentation in vchiq_dump_* functions
> staging: vchiq: Have vchiq_dump_* functions return an error code
>
> .../interface/vchiq_arm/vchiq_2835_arm.c | 5 +-
> .../interface/vchiq_arm/vchiq_arm.c | 129 ++++++++++--------
> .../interface/vchiq_arm/vchiq_core.c | 104 ++++++++++----
> .../interface/vchiq_arm/vchiq_core.h | 12 +-
> 4 files changed, 153 insertions(+), 97 deletions(-)
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/4] staging: vchiq: Fix vchiq_read return value in case of error
2019-11-20 20:20 ` [PATCH v2 0/4] staging: vchiq: Fix vchiq_read return value in case of error Marcelo Diop-Gonzalez
` (3 preceding siblings ...)
2019-11-20 20:21 ` [PATCH v2 4/4] staging: vchiq: Have vchiq_dump_* functions return an error code Marcelo Diop-Gonzalez
@ 2019-11-21 19:38 ` Nicolas Saenz Julienne
2019-11-21 21:24 ` Marcelo Diop-Gonzalez
4 siblings, 1 reply; 16+ messages in thread
From: Nicolas Saenz Julienne @ 2019-11-21 19:38 UTC (permalink / raw)
To: Marcelo Diop-Gonzalez, gregkh, eric, wahrenst; +Cc: devel, linux-rpi-kernel
[-- Attachment #1.1: Type: text/plain, Size: 2400 bytes --]
Hi Marcelo,
On Wed, 2019-11-20 at 15:20 -0500, Marcelo Diop-Gonzalez wrote:
> This is a proposed fix of an issue regarding the handling of positive
> return values from copy_to_user() in vchiq_read(). It looks like the
> struct dump_context passed to the vchiq_dump() function keeps track
> of the number of bytes written in the context->actual field. When
> copy_to_user() returns a positive value, this is set to -EFAULT. The
> problem is that this is never returned to the user, and instead the
> function continues, adding the number of bytes that should have
> been copied to context->actual.
>
> Running the following program:
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <fcntl.h>
> #include <unistd.h>
> #include <errno.h>
>
> int main(int argc, char **argv) {
> int fd = open("/dev/vchiq", 0);
> if (fd < 0)
> exit(-1);
> sbrk(getpagesize());
> char *bad_buf = sbrk(0)-100;
> int x = read(fd, bad_buf, 2000);
> printf("%d %d\n", x, errno);
> puts(bad_buf);
> }
>
> gives this output:
>
> -1 1
> State 0: CONNECTED
> tx_po
>
>
> Remote: slots 2-32 tx_pos=578 recycle=1f
> Slots claimed:
>
> Note the mangled output and incorrect errno value. Messing with the
> constants in that toy program changes the results. Sometimes read()
> returns with no error, sometimes it returns with a wrong error code,
> sometimes with the right one. But it seems that it only ever returns an
> error at all accidentally, due to the fact that the comparison between
> context->actual and context->space in vchiq_dump() is unsigned, so that
> that function won't do anything else if it ever sets context->actual
> to a negative value.
>
> After this patchset, the above program prints this:
>
> -1 14
> State 0: CONNECTED
> tx_pos=b4a218(@165de6b4), rx_pos=ae0668(@f02b54f4)
> Version: 8 (min 3)
> Stats
>
> Help with testing would be appreciated. So far I've basically just
> diffed the output of 'cat /dev/vchiq', run the program above with
> a few different values, and run vchiq_test a few times.
>
> These were applied to the staging-next branch of the tree
> at git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
For the whole series:
Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Thanks,
Nicolas
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 169 bytes --]
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] staging: vchiq: Fix vchiq_read return value in case of error
2019-11-21 10:42 ` [PATCH 0/3] " Stefan Wahren
@ 2019-11-21 19:41 ` Nicolas Saenz Julienne
0 siblings, 0 replies; 16+ messages in thread
From: Nicolas Saenz Julienne @ 2019-11-21 19:41 UTC (permalink / raw)
To: Stefan Wahren, Marcelo Diop-Gonzalez, gregkh, eric
Cc: devel, Phil Elwell, linux-rpi-kernel
[-- Attachment #1.1: Type: text/plain, Size: 452 bytes --]
On Thu, 2019-11-21 at 11:42 +0100, Stefan Wahren wrote:
[...]
> > Help with testing would be appreciated. So far I've basically just
> > diffed the output of 'cat /dev/vchiq', run the program above with
> > a few different values, and run vchiq_test a few times.
>
> i consider this as sufficient, but i'm not the VCHIQ expert.
Agree, the patch only affects the vchiq state dump code, I think it's OK with
this test.
Regards,
Nicolas
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 169 bytes --]
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 0/4] staging: vchiq: Fix vchiq_read return value in case of error
2019-11-21 19:38 ` [PATCH v2 0/4] staging: vchiq: Fix vchiq_read return value in case of error Nicolas Saenz Julienne
@ 2019-11-21 21:24 ` Marcelo Diop-Gonzalez
0 siblings, 0 replies; 16+ messages in thread
From: Marcelo Diop-Gonzalez @ 2019-11-21 21:24 UTC (permalink / raw)
To: Nicolas Saenz Julienne
Cc: devel, Greg KH, linux-rpi-kernel, Stefan Wahren, eric
On Thu, Nov 21, 2019 at 2:39 PM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> Hi Marcelo,
>
> On Wed, 2019-11-20 at 15:20 -0500, Marcelo Diop-Gonzalez wrote:
> > This is a proposed fix of an issue regarding the handling of positive
> > return values from copy_to_user() in vchiq_read(). It looks like the
> > struct dump_context passed to the vchiq_dump() function keeps track
> > of the number of bytes written in the context->actual field. When
> > copy_to_user() returns a positive value, this is set to -EFAULT. The
> > problem is that this is never returned to the user, and instead the
> > function continues, adding the number of bytes that should have
> > been copied to context->actual.
> >
> > Running the following program:
> >
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <fcntl.h>
> > #include <unistd.h>
> > #include <errno.h>
> >
> > int main(int argc, char **argv) {
> > int fd = open("/dev/vchiq", 0);
> > if (fd < 0)
> > exit(-1);
> > sbrk(getpagesize());
> > char *bad_buf = sbrk(0)-100;
> > int x = read(fd, bad_buf, 2000);
> > printf("%d %d\n", x, errno);
> > puts(bad_buf);
> > }
> >
> > gives this output:
> >
> > -1 1
> > State 0: CONNECTED
> > tx_po
> >
> >
> > Remote: slots 2-32 tx_pos=578 recycle=1f
> > Slots claimed:
> >
> > Note the mangled output and incorrect errno value. Messing with the
> > constants in that toy program changes the results. Sometimes read()
> > returns with no error, sometimes it returns with a wrong error code,
> > sometimes with the right one. But it seems that it only ever returns an
> > error at all accidentally, due to the fact that the comparison between
> > context->actual and context->space in vchiq_dump() is unsigned, so that
> > that function won't do anything else if it ever sets context->actual
> > to a negative value.
> >
> > After this patchset, the above program prints this:
> >
> > -1 14
> > State 0: CONNECTED
> > tx_pos=b4a218(@165de6b4), rx_pos=ae0668(@f02b54f4)
> > Version: 8 (min 3)
> > Stats
> >
> > Help with testing would be appreciated. So far I've basically just
> > diffed the output of 'cat /dev/vchiq', run the program above with
> > a few different values, and run vchiq_test a few times.
> >
> > These were applied to the staging-next branch of the tree
> > at git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
>
> For the whole series:
>
> Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Thanks for reviewing!
-Marcelo
>
> Thanks,
> Nicolas
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] staging: vchiq: Have vchiq_dump_* functions return an error code
2019-11-20 18:50 ` Dan Carpenter
@ 2019-11-21 21:25 ` Marcelo Diop-Gonzalez
0 siblings, 0 replies; 16+ messages in thread
From: Marcelo Diop-Gonzalez @ 2019-11-21 21:25 UTC (permalink / raw)
To: Dan Carpenter; +Cc: devel, Greg KH, linux-rpi-kernel, Stefan Wahren, eric
On Wed, Nov 20, 2019 at 1:51 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> Thanks for the patch. Looks good.
>
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Appreciate the review!
-Marcelo
>
> On Wed, Nov 20, 2019 at 10:36:48AM -0500, Marcelo Diop-Gonzalez wrote:
> > copy_bytes = min_t(int, len, context->space - context->actual);
> > if (copy_bytes == 0)
> > - return;
> > + return 0;
> > if (copy_to_user(context->buf + context->actual, str,
> > copy_bytes))
> > - context->actual = -EFAULT;
> ^^^^^^^^^^^^^^^^^^^^^^^^^
>
> > + return -EFAULT;
> > context->actual += copy_bytes;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> In the original code we saved the error code to context->actual, then
> we added some bytes
>
> > len -= copy_bytes;
> >
> > @@ -2109,8 +2108,9 @@ vchiq_dump(void *dump_context, const char *str, int len)
> >
> > if (copy_to_user(context->buf + context->actual - 1,
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> and then we tried to use the result to corrupt the user space program.
> So this patch is a nice bug fix.
>
> > &cr, 1))
> > - context->actual = -EFAULT;
> > + return -EFAULT;
> > }
> > + return 0;
> > }
>
> regards,
> dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-11-21 21:25 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-20 15:36 [PATCH 0/3] staging: vchiq: Fix vchiq_read return value in case of error Marcelo Diop-Gonzalez
2019-11-20 15:36 ` [PATCH 1/3] staging: vchiq_dump: Replace min with min_t Marcelo Diop-Gonzalez
2019-11-20 15:46 ` Greg KH
2019-11-20 15:36 ` [PATCH 2/3] staging: vchiq: Refactor indentation in vchiq_dump_* functions Marcelo Diop-Gonzalez
2019-11-20 15:36 ` [PATCH 3/3] staging: vchiq: Have vchiq_dump_* functions return an error code Marcelo Diop-Gonzalez
2019-11-20 18:50 ` Dan Carpenter
2019-11-21 21:25 ` Marcelo Diop-Gonzalez
2019-11-20 20:20 ` [PATCH v2 0/4] staging: vchiq: Fix vchiq_read return value in case of error Marcelo Diop-Gonzalez
2019-11-20 20:20 ` [PATCH v2 1/4] staging: vchiq: Fix block comment format in vchiq_dump() Marcelo Diop-Gonzalez
2019-11-20 20:21 ` [PATCH v2 2/4] staging: vchiq_dump: Replace min with min_t Marcelo Diop-Gonzalez
2019-11-20 20:21 ` [PATCH v2 3/4] staging: vchiq: Refactor indentation in vchiq_dump_* functions Marcelo Diop-Gonzalez
2019-11-20 20:21 ` [PATCH v2 4/4] staging: vchiq: Have vchiq_dump_* functions return an error code Marcelo Diop-Gonzalez
2019-11-21 19:38 ` [PATCH v2 0/4] staging: vchiq: Fix vchiq_read return value in case of error Nicolas Saenz Julienne
2019-11-21 21:24 ` Marcelo Diop-Gonzalez
2019-11-21 10:42 ` [PATCH 0/3] " Stefan Wahren
2019-11-21 19:41 ` Nicolas Saenz Julienne
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).