Re: [PATCH 1/2] staging: vchiq: Fix bulk userdata handling
On Mon, Jan 04, 2021 at 07:26:42PM +, Phil Elwell wrote:
> On 04/01/2021 18:31, Dan Carpenter wrote:
> > On Mon, Jan 04, 2021 at 12:09:27PM +, Phil Elwell wrote:
> > > The addition of the local 'userdata' pointer to
> > > vchiq_irq_queue_bulk_tx_rx omitted the case where neither BLOCKING nor
> > > WAITING modes are used, in which case the value provided by the
> > > caller is replaced with a NULL.
> > >
> > > Fixes: 4184da4f316a ("staging: vchiq: fix __user annotations")
> > >
> > > Signed-off-by: Phil Elwell
> > > ---
> > > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > 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 f500a7043805..2a8883673ba1 100644
> > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > @@ -958,7 +958,7 @@ static int vchiq_irq_queue_bulk_tx_rx(struct
> > > vchiq_instance *instance,
> > > struct vchiq_service *service;
> > > struct bulk_waiter_node *waiter = NULL;
> > > bool found = false;
> > > - void *userdata = NULL;
> > > + void *userdata;
> > > int status = 0;
> > > int ret;
> > > @@ -997,6 +997,8 @@ static int vchiq_irq_queue_bulk_tx_rx(struct
> > > vchiq_instance *instance,
> > > "found bulk_waiter %pK for pid %d", waiter,
> > > current->pid);
> > > userdata = &waiter->bulk_waiter;
> > > + } else {
> > > + userdata = args->userdata;
> >
> > "args->userdata" is marked as a user pointer so we really don't want to
> > mix user and kernel pointers here. Presumably this opens up a large
> > security hole.
>
> It's an opaque, pointer-sized token that only exists to bereturned to
> userspace (or not,
> without this patch) - it's hard to see that as a security hole.
I was assuming the bug here was a NULL dereference... Apparently that's
not the case? The commit message needs to be updated to be more clear
about how the bug looks like to the user.
Are we using the "&waiter->bulk_waiter" as a "token to be returned to
userspace" as well? It looks like maybe it is in vchiq_put_completion().
That defeats KASLR and is a different sort of security problem.
Mixing __user pointers and regular pointers is dangerous and has lead to
security problems in this driver in the past. But also mixing mixing
tokens with pointers just makes the code hard to read. Instead of
undoing Arnd's work where he split the user space and kernel pointers
apart we should go ahead and spit it up even more. At least add a giant
FIXME comment and an item in the TODO list so we don't forget to do this
before removing the code from staging.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: android: ashmem: Declared file operation with const keyword
On Mon, Dec 28, 2020 at 12:13:00AM -0500, jovin555 wrote: > Warning found by checkpatch.pl script. > > Signed-off-by: jovin555 Your Mama didn't name you "jovin555". You need to use your real name like signing a legal document. Same for the "From:" header. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: vchiq: Fix bulk userdata handling
On Tue, 5 Jan 2021 at 11:04, Dan Carpenter wrote:
>
> On Mon, Jan 04, 2021 at 07:26:42PM +, Phil Elwell wrote:
> > On 04/01/2021 18:31, Dan Carpenter wrote:
> > > On Mon, Jan 04, 2021 at 12:09:27PM +, Phil Elwell wrote:
> > > > The addition of the local 'userdata' pointer to
> > > > vchiq_irq_queue_bulk_tx_rx omitted the case where neither BLOCKING nor
> > > > WAITING modes are used, in which case the value provided by the
> > > > caller is replaced with a NULL.
> > > >
> > > > Fixes: 4184da4f316a ("staging: vchiq: fix __user annotations")
> > > >
> > > > Signed-off-by: Phil Elwell
> > > > ---
> > > > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 +++-
> > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > 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 f500a7043805..2a8883673ba1 100644
> > > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > @@ -958,7 +958,7 @@ static int vchiq_irq_queue_bulk_tx_rx(struct
> > > > vchiq_instance *instance,
> > > > struct vchiq_service *service;
> > > > struct bulk_waiter_node *waiter = NULL;
> > > > bool found = false;
> > > > - void *userdata = NULL;
> > > > + void *userdata;
> > > > int status = 0;
> > > > int ret;
> > > > @@ -997,6 +997,8 @@ static int vchiq_irq_queue_bulk_tx_rx(struct
> > > > vchiq_instance *instance,
> > > > "found bulk_waiter %pK for pid %d", waiter,
> > > > current->pid);
> > > > userdata = &waiter->bulk_waiter;
> > > > + } else {
> > > > + userdata = args->userdata;
> > >
> > > "args->userdata" is marked as a user pointer so we really don't want to
> > > mix user and kernel pointers here. Presumably this opens up a large
> > > security hole.
> >
> > It's an opaque, pointer-sized token that only exists to bereturned to
> > userspace (or not,
> > without this patch) - it's hard to see that as a security hole.
>
> I was assuming the bug here was a NULL dereference... Apparently that's
> not the case? The commit message needs to be updated to be more clear
> about how the bug looks like to the user.
>
> Are we using the "&waiter->bulk_waiter" as a "token to be returned to
> userspace" as well? It looks like maybe it is in vchiq_put_completion().
> That defeats KASLR and is a different sort of security problem.
>
> Mixing __user pointers and regular pointers is dangerous and has lead to
> security problems in this driver in the past. But also mixing mixing
> tokens with pointers just makes the code hard to read. Instead of
> undoing Arnd's work where he split the user space and kernel pointers
> apart we should go ahead and spit it up even more. At least add a giant
> FIXME comment and an item in the TODO list so we don't forget to do this
> before removing the code from staging.
Those all sound like valid comments to have made against the original
patch, but that seems to have received little attention.
I'll just leave this here - perhaps Arnd has the patience to finish the job.
Phil
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: vchiq: delete obselete comment
This comment describes a security problem which was fixed in commit
1c954540c0eb ("staging: vchiq: avoid mixing kernel and user pointers").
The bug is fixed now so the FIXME can be removed.
Signed-off-by: Dan Carpenter
---
.../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 7 ---
1 file changed, 7 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 f500a7043805..54770a9b4735 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -999,13 +999,6 @@ static int vchiq_irq_queue_bulk_tx_rx(struct
vchiq_instance *instance,
userdata = &waiter->bulk_waiter;
}
- /*
-* FIXME address space mismatch:
-* args->data may be interpreted as a kernel pointer
-* in create_pagelist() called from vchiq_bulk_transfer(),
-* accessing kernel data instead of user space, based on the
-* address.
-*/
status = vchiq_bulk_transfer(args->handle, NULL, args->data, args->size,
userdata, args->mode, dir);
--
2.29.2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: vchiq: Fix bulk userdata handling
On Tue, Jan 05, 2021 at 11:53:32AM +, Phil Elwell wrote:
> On Tue, 5 Jan 2021 at 11:04, Dan Carpenter wrote:
> >
> > On Mon, Jan 04, 2021 at 07:26:42PM +, Phil Elwell wrote:
> > > On 04/01/2021 18:31, Dan Carpenter wrote:
> > > > On Mon, Jan 04, 2021 at 12:09:27PM +, Phil Elwell wrote:
> > > > > The addition of the local 'userdata' pointer to
> > > > > vchiq_irq_queue_bulk_tx_rx omitted the case where neither BLOCKING nor
> > > > > WAITING modes are used, in which case the value provided by the
> > > > > caller is replaced with a NULL.
> > > > >
> > > > > Fixes: 4184da4f316a ("staging: vchiq: fix __user annotations")
> > > > >
> > > > > Signed-off-by: Phil Elwell
> > > > > ---
> > > > > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4
> > > > > +++-
> > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > 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 f500a7043805..2a8883673ba1 100644
> > > > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > > @@ -958,7 +958,7 @@ static int vchiq_irq_queue_bulk_tx_rx(struct
> > > > > vchiq_instance *instance,
> > > > > struct vchiq_service *service;
> > > > > struct bulk_waiter_node *waiter = NULL;
> > > > > bool found = false;
> > > > > - void *userdata = NULL;
> > > > > + void *userdata;
> > > > > int status = 0;
> > > > > int ret;
> > > > > @@ -997,6 +997,8 @@ static int vchiq_irq_queue_bulk_tx_rx(struct
> > > > > vchiq_instance *instance,
> > > > > "found bulk_waiter %pK for pid %d", waiter,
> > > > > current->pid);
> > > > > userdata = &waiter->bulk_waiter;
> > > > > + } else {
> > > > > + userdata = args->userdata;
> > > >
> > > > "args->userdata" is marked as a user pointer so we really don't want to
> > > > mix user and kernel pointers here. Presumably this opens up a large
> > > > security hole.
> > >
> > > It's an opaque, pointer-sized token that only exists to bereturned to
> > > userspace (or not,
> > > without this patch) - it's hard to see that as a security hole.
> >
> > I was assuming the bug here was a NULL dereference... Apparently that's
> > not the case? The commit message needs to be updated to be more clear
> > about how the bug looks like to the user.
> >
> > Are we using the "&waiter->bulk_waiter" as a "token to be returned to
> > userspace" as well? It looks like maybe it is in vchiq_put_completion().
> > That defeats KASLR and is a different sort of security problem.
> >
> > Mixing __user pointers and regular pointers is dangerous and has lead to
> > security problems in this driver in the past. But also mixing mixing
> > tokens with pointers just makes the code hard to read. Instead of
> > undoing Arnd's work where he split the user space and kernel pointers
> > apart we should go ahead and spit it up even more. At least add a giant
> > FIXME comment and an item in the TODO list so we don't forget to do this
> > before removing the code from staging.
>
> Those all sound like valid comments to have made against the original
> patch, but that seems to have received little attention.
>
> I'll just leave this here - perhaps Arnd has the patience to finish the job.
I kind of have a headache today so maybe I shouldn't be sending emails.
But really, all I'm asking is for is two fairly reasonable things:
1) The commit message needs to say what the bug looks like to the user.
Up to now, I still have no idea the answer to this question.
2) Put a note in the TODO which says: "Clean up Sparse warnings from
__user annotations. See vchiq_irq_queue_bulk_tx_rx(). Ensure that
the the address of "&waiter->bulk_waiter" is never disclosed to
userspace."
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: vchiq: fix uninitialized variable copy
From: Arnd Bergmann
Smatch found a local variable that can get copied to another
local variable without an initializion in the error case:
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c:1056
vchiq_get_user_ptr() error: uninitialized symbol 'ptr'.
This seems harmless, as the function should normally get inlined, with
the output directly written or not. In any case, the uninitialized data
is never used after get_user() fails.
As Dan mentions, it could still trigger an UBSAN runtime error, and it
is of course a bad idea to copy uninitialized variables, so just
bail out early.
Reported-by: kernel test robot
Reported-by: Dan Carpenter
Signed-off-by: Arnd Bergmann
---
.../vc04_services/interface/vchiq_arm/vchiq_arm.c| 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
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 f500a7043805..63a0045ef9c5 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1057,14 +1057,21 @@ static inline int vchiq_get_user_ptr(void __user **buf,
void __user *ubuf, int i
compat_uptr_t ptr32;
compat_uptr_t __user *uptr = ubuf;
ret = get_user(ptr32, uptr + index);
+ if (ret)
+ return ret;
+
*buf = compat_ptr(ptr32);
} else {
uintptr_t ptr, __user *uptr = ubuf;
ret = get_user(ptr, uptr + index);
+
+ if (ret)
+ return ret;
+
*buf = (void __user *)ptr;
}
- return ret;
+ return 0;
}
struct vchiq_completion_data32 {
--
2.29.2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/2] drm/bridge: anx7625: add MIPI DPI input feature support
On Thu, Dec 31, 2020 at 10:22:36AM +0800, Xin Ji wrote:
> static int anx7625_read_ctrl_status_p0(struct anx7625_data *ctx)
> {
> return anx7625_reg_read(ctx, ctx->i2c.rx_p0_client, AP_AUX_CTRL_STATUS);
> @@ -189,10 +203,64 @@ static int wait_aux_op_finish(struct anx7625_data *ctx)
> AP_AUX_CTRL_STATUS);
> if (val < 0 || (val & 0x0F)) {
> DRM_DEV_ERROR(dev, "aux status %02x\n", val);
> - val = -EIO;
> + return -EIO;
> }
>
> - return val;
> + return 0;
This s/val/0/ change seems like a bug fix. Could you please send that
as a separate patch at the start of the patch set?
> +}
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: vchiq: delete obselete comment
On Tue, Jan 5, 2021 at 2:19 PM Dan Carpenter wrote:
>
> This comment describes a security problem which was fixed in commit
> 1c954540c0eb ("staging: vchiq: avoid mixing kernel and user pointers").
> The bug is fixed now so the FIXME can be removed.
>
> Signed-off-by: Dan Carpenter
Reviewed-by: Arnd Bergmann
There is still another sparse warning for a remaining __user address
space mismatch in the driver, but this one seems to be fixed as you
say. Thanks for the fix!
Arnd
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: vchiq: Fix bulk userdata handling
On Tue, Jan 5, 2021 at 12:53 PM Phil Elwell wrote: > On Tue, 5 Jan 2021 at 11:04, Dan Carpenter wrote: > > > > Mixing __user pointers and regular pointers is dangerous and has lead to > > security problems in this driver in the past. But also mixing mixing > > tokens with pointers just makes the code hard to read. Instead of > > undoing Arnd's work where he split the user space and kernel pointers > > apart we should go ahead and spit it up even more. At least add a giant > > FIXME comment and an item in the TODO list so we don't forget to do this > > before removing the code from staging. > > Those all sound like valid comments to have made against the original > patch, but that seems to have received little attention. > > I'll just leave this here - perhaps Arnd has the patience to finish the job. I don't really have an interest in this driver. I did a larger cleanup in order to kill off copy_in_user() from the kernel, and then cleaned it up some more for good measure, but I would hope someone else can finish the address space mismatch. Arnd ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/2] staging: greybus: vibrator: use proper API for vibrator devices
The correct user/kernel api for vibrator devices is the Input rumble
api, not a random sysfs file like the greybus vibrator driver currently
uses.
Add support for the correct input api to the vibrator driver so that it
hooks up to the kernel and userspace correctly.
Cc: Johan Hovold
Cc: Alex Elder
Signed-off-by: Greg Kroah-Hartman
---
drivers/staging/greybus/vibrator.c | 59 ++
1 file changed, 59 insertions(+)
diff --git a/drivers/staging/greybus/vibrator.c
b/drivers/staging/greybus/vibrator.c
index 0e2b188e5ca3..94110cadb5bd 100644
--- a/drivers/staging/greybus/vibrator.c
+++ b/drivers/staging/greybus/vibrator.c
@@ -13,13 +13,18 @@
#include
#include
#include
+#include
#include
struct gb_vibrator_device {
struct gb_connection*connection;
+ struct input_dev*input;
struct device *dev;
int minor; /* vibrator minor number */
struct delayed_work delayed_work;
+ boolrunning;
+ boolon;
+ struct work_struct play_work;
};
/* Greybus Vibrator operation types */
@@ -36,6 +41,7 @@ static int turn_off(struct gb_vibrator_device *vib)
gb_pm_runtime_put_autosuspend(bundle);
+ vib->on = false;
return ret;
}
@@ -59,11 +65,48 @@ static int turn_on(struct gb_vibrator_device *vib, u16
timeout_ms)
return ret;
}
+ vib->on = true;
schedule_delayed_work(&vib->delayed_work, msecs_to_jiffies(timeout_ms));
return 0;
}
+static void gb_vibrator_play_work(struct work_struct *work)
+{
+ struct gb_vibrator_device *vib =
+ container_of(work, struct gb_vibrator_device, play_work);
+
+ if (vib->running)
+ turn_off(vib);
+ else
+ turn_on(vib, 100);
+}
+
+static int gb_vibrator_play_effect(struct input_dev *input, void *data,
+ struct ff_effect *effect)
+{
+ struct gb_vibrator_device *vib = input_get_drvdata(input);
+ int level;
+
+ level = effect->u.rumble.strong_magnitude;
+ if (!level)
+ level = effect->u.rumble.weak_magnitude;
+
+ vib->running = level;
+ schedule_work(&vib->play_work);
+ return 0;
+}
+
+static void gb_vibrator_close(struct input_dev *input)
+{
+ struct gb_vibrator_device *vib = input_get_drvdata(input);
+
+ cancel_delayed_work_sync(&vib->delayed_work);
+ cancel_work_sync(&vib->play_work);
+ turn_off(vib);
+ vib->running = false;
+}
+
static void gb_vibrator_worker(struct work_struct *work)
{
struct delayed_work *delayed_work = to_delayed_work(work);
@@ -169,10 +212,26 @@ static int gb_vibrator_probe(struct gb_bundle *bundle,
INIT_DELAYED_WORK(&vib->delayed_work, gb_vibrator_worker);
+ INIT_WORK(&vib->play_work, gb_vibrator_play_work);
+ vib->input->name = "greybus-vibrator";
+ vib->input->close = gb_vibrator_close;
+ vib->input->dev.parent = &bundle->dev;
+ vib->input->id.bustype = BUS_HOST;
+
+ input_set_drvdata(vib->input, vib);
+ input_set_capability(vib->input, EV_FF, FF_RUMBLE);
+
+ retval = input_ff_create_memless(vib->input, NULL,
+gb_vibrator_play_effect);
+ if (retval)
+ goto err_device_remove;
+
gb_pm_runtime_put_autosuspend(bundle);
return 0;
+err_device_remove:
+ device_unregister(vib->dev);
err_ida_remove:
ida_simple_remove(&minors, vib->minor);
err_connection_disable:
--
2.30.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/2] greybus: vibrator: rip out custom sysfs api
No need for a custom sysfs api for the greybus vibrator driver now that
it is hooked up to the kernel's input layer, so rip it out.
Cc: Johan Hovold
Cc: Alex Elder
Signed-off-by: Greg Kroah-Hartman
---
drivers/staging/greybus/vibrator.c | 125 ++---
1 file changed, 5 insertions(+), 120 deletions(-)
diff --git a/drivers/staging/greybus/vibrator.c
b/drivers/staging/greybus/vibrator.c
index 94110cadb5bd..d93c8f7e1bd6 100644
--- a/drivers/staging/greybus/vibrator.c
+++ b/drivers/staging/greybus/vibrator.c
@@ -19,9 +19,6 @@
struct gb_vibrator_device {
struct gb_connection*connection;
struct input_dev*input;
- struct device *dev;
- int minor; /* vibrator minor number */
- struct delayed_work delayed_work;
boolrunning;
boolon;
struct work_struct play_work;
@@ -45,7 +42,7 @@ static int turn_off(struct gb_vibrator_device *vib)
return ret;
}
-static int turn_on(struct gb_vibrator_device *vib, u16 timeout_ms)
+static int turn_on(struct gb_vibrator_device *vib)
{
struct gb_bundle *bundle = vib->connection->bundle;
int ret;
@@ -54,10 +51,6 @@ static int turn_on(struct gb_vibrator_device *vib, u16
timeout_ms)
if (ret)
return ret;
- /* Vibrator was switched ON earlier */
- if (cancel_delayed_work_sync(&vib->delayed_work))
- turn_off(vib);
-
ret = gb_operation_sync(vib->connection, GB_VIBRATOR_TYPE_ON,
NULL, 0, NULL, 0);
if (ret) {
@@ -66,8 +59,6 @@ static int turn_on(struct gb_vibrator_device *vib, u16
timeout_ms)
}
vib->on = true;
- schedule_delayed_work(&vib->delayed_work, msecs_to_jiffies(timeout_ms));
-
return 0;
}
@@ -79,7 +70,7 @@ static void gb_vibrator_play_work(struct work_struct *work)
if (vib->running)
turn_off(vib);
else
- turn_on(vib, 100);
+ turn_on(vib);
}
static int gb_vibrator_play_effect(struct input_dev *input, void *data,
@@ -101,68 +92,17 @@ static void gb_vibrator_close(struct input_dev *input)
{
struct gb_vibrator_device *vib = input_get_drvdata(input);
- cancel_delayed_work_sync(&vib->delayed_work);
cancel_work_sync(&vib->play_work);
turn_off(vib);
vib->running = false;
}
-static void gb_vibrator_worker(struct work_struct *work)
-{
- struct delayed_work *delayed_work = to_delayed_work(work);
- struct gb_vibrator_device *vib =
- container_of(delayed_work,
-struct gb_vibrator_device,
-delayed_work);
-
- turn_off(vib);
-}
-
-static ssize_t timeout_store(struct device *dev, struct device_attribute *attr,
-const char *buf, size_t count)
-{
- struct gb_vibrator_device *vib = dev_get_drvdata(dev);
- unsigned long val;
- int retval;
-
- retval = kstrtoul(buf, 10, &val);
- if (retval < 0) {
- dev_err(dev, "could not parse timeout value %d\n", retval);
- return retval;
- }
-
- if (val)
- retval = turn_on(vib, (u16)val);
- else
- retval = turn_off(vib);
- if (retval)
- return retval;
-
- return count;
-}
-static DEVICE_ATTR_WO(timeout);
-
-static struct attribute *vibrator_attrs[] = {
- &dev_attr_timeout.attr,
- NULL,
-};
-ATTRIBUTE_GROUPS(vibrator);
-
-static struct class vibrator_class = {
- .name = "vibrator",
- .owner = THIS_MODULE,
- .dev_groups = vibrator_groups,
-};
-
-static DEFINE_IDA(minors);
-
static int gb_vibrator_probe(struct gb_bundle *bundle,
const struct greybus_bundle_id *id)
{
struct greybus_descriptor_cport *cport_desc;
struct gb_connection *connection;
struct gb_vibrator_device *vib;
- struct device *dev;
int retval;
if (bundle->num_cports != 1)
@@ -192,26 +132,6 @@ static int gb_vibrator_probe(struct gb_bundle *bundle,
if (retval)
goto err_connection_destroy;
- /*
-* For now we create a device in sysfs for the vibrator, but odds are
-* there is a "real" device somewhere in the kernel for this, but I
-* can't find it at the moment...
-*/
- vib->minor = ida_simple_get(&minors, 0, 0, GFP_KERNEL);
- if (vib->minor < 0) {
- retval = vib->minor;
- goto err_connection_disable;
- }
- dev = device_create(&vibrator_class, &bundle->dev,
- MKDEV(0, 0), vib, "vibrator%d", vib->minor);
- if (IS_ERR(dev)) {
- retval = -EINVAL;
- goto err_ida_remove;
- }
- vib->dev = dev;
-
- INIT_DELAYE
[PATCH v2 0/3] A trio of vchiq bulk transfer fixes
The recent batch of vchiq improvements broke bulk transfers in two ways: 1. The userdata associated with a transfer was lost in the case that a non-blocking mode was used. 2. The 64-bit ioctl compatibility shim for a bulk transfer used the wrong ioctl command. This patch set fixes both of those bugs, and adds a security-related note to the TODO file. Changes in v2: - Expand the commit message on patch 1 to clarify the impact of the bug, and add Tested-by. - Add commit 3 with an additional TODO item. - Change the name of the patch set to be numerically accurate. Phil Elwell (3): staging: vchiq: Fix bulk userdata handling staging: vchiq: Fix bulk transfers on 64-bit builds staging: vc04_services: Add a note to the TODO drivers/staging/vc04_services/interface/TODO| 4 .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 6 -- 2 files changed, 8 insertions(+), 2 deletions(-) -- 2.25.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 1/3] staging: vchiq: Fix bulk userdata handling
The addition of the local 'userdata' pointer to
vchiq_irq_queue_bulk_tx_rx omitted the case where neither BLOCKING nor
WAITING modes are used, in which case the value provided by the
caller is not returned to them as expected, but instead it is replaced
with a NULL. This lack of a suitable context may cause the application
to crash or otherwise malfunction.
Fixes: 4184da4f316a ("staging: vchiq: fix __user annotations")
Signed-off-by: Phil Elwell
Tested-by: Stefan Wahren
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
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 f500a7043805..2a8883673ba1 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -958,7 +958,7 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance
*instance,
struct vchiq_service *service;
struct bulk_waiter_node *waiter = NULL;
bool found = false;
- void *userdata = NULL;
+ void *userdata;
int status = 0;
int ret;
@@ -997,6 +997,8 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance
*instance,
"found bulk_waiter %pK for pid %d", waiter,
current->pid);
userdata = &waiter->bulk_waiter;
+ } else {
+ userdata = args->userdata;
}
/*
--
2.25.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 3/3] staging: vc04_services: Add a note to the TODO
Record in the TODO file that the address of "&waiter->bulk_waiter" should never be returned to userspace. Signed-off-by: Phil Elwell --- drivers/staging/vc04_services/interface/TODO | 4 1 file changed, 4 insertions(+) diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO index fc2752bc95b2..0bcb8f158afc 100644 --- a/drivers/staging/vc04_services/interface/TODO +++ b/drivers/staging/vc04_services/interface/TODO @@ -91,3 +91,7 @@ The first thing one generally sees in a probe function is a memory allocation for all the device specific data. This structure is then passed all over the driver. This is good practice since it makes the driver work regardless of the number of devices probed. + +14) Clean up Sparse warnings from __user annotations. See +vchiq_irq_queue_bulk_tx_rx(). Ensure that the address of "&waiter->bulk_waiter" +is never disclosed to userspace. -- 2.25.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 2/3] staging: vchiq: Fix bulk transfers on 64-bit builds
The recent change to the bulk transfer compat function missed the fact
the relevant ioctl command is VCHIQ_IOC_QUEUE_BULK_TRANSMIT32, not
VCHIQ_IOC_QUEUE_BULK_TRANSMIT, as any attempt to send a bulk block
to the VPU would have shown.
Fixes: a4367cd2b231 ("staging: vchiq: convert compat bulk transfer")
Signed-off-by: Phil Elwell
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 2a8883673ba1..2ca5805b2fce 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -1717,7 +1717,7 @@ vchiq_compat_ioctl_queue_bulk(struct file *file,
{
struct vchiq_queue_bulk_transfer32 args32;
struct vchiq_queue_bulk_transfer args;
- enum vchiq_bulk_dir dir = (cmd == VCHIQ_IOC_QUEUE_BULK_TRANSMIT) ?
+ enum vchiq_bulk_dir dir = (cmd == VCHIQ_IOC_QUEUE_BULK_TRANSMIT32) ?
VCHIQ_BULK_TRANSMIT : VCHIQ_BULK_RECEIVE;
if (copy_from_user(&args32, argp, sizeof(args32)))
--
2.25.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 0/3] A trio of vchiq bulk transfer fixes
Thanks! Acked-by: Dan Carpenter regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 00/48] Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs
On Thu, Dec 17, 2020 at 09:05:50PM +0300, Dmitry Osipenko wrote: > Introduce core voltage scaling for NVIDIA Tegra20/30 SoCs, which reduces > power consumption and heating of the Tegra chips. Tegra SoC has multiple > hardware units which belong to a core power domain of the SoC and share > the core voltage. The voltage must be selected in accordance to a minimum > requirement of every core hardware unit. > > The minimum core voltage requirement depends on: > > 1. Clock enable state of a hardware unit. > 2. Clock frequency. > 3. Unit's internal idling/active state. > > This series is tested on Acer A500 (T20), AC100 (T20), Nexus 7 (T30), > Ouya (T30), TK1 (T124) and some others. I also added voltage scaling to > the Ventana (T20) and Cardhu (T30) boards which are tested by NVIDIA's CI > farm. Tegra30 is now couple degrees cooler on Nexus 7 and stays cool on > Ouya (instead of becoming burning hot) while system is idling. It should > be possible to improve this further by implementing a more advanced power > management features for the kernel drivers. > > The DVFS support is opt-in for all boards, meaning that older DTBs will > continue to work like they did it before this series. It should be possible > to easily add the core voltage scaling support for Tegra114+ SoCs based on > this grounding work later on, if anyone will want to implement it. The same comment as for your interconnect work: for sets touching multiple systems please mention the dependencies between patches in the cover letter. Not as a reply to such remark like I make here, but as a separate entry in the cover letter. Best regards, Krzysztof ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/3] staging: vchiq: Fix bulk transfers on 64-bit builds
On Tue, Jan 5, 2021 at 5:20 PM Phil Elwell wrote:
>
> The recent change to the bulk transfer compat function missed the fact
> the relevant ioctl command is VCHIQ_IOC_QUEUE_BULK_TRANSMIT32, not
> VCHIQ_IOC_QUEUE_BULK_TRANSMIT, as any attempt to send a bulk block
> to the VPU would have shown.
>
> Fixes: a4367cd2b231 ("staging: vchiq: convert compat bulk transfer")
>
> Signed-off-by: Phil Elwell
Acked-by: Arnd Bergmann
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: greybus: vibrator: use proper API for vibrator devices
Hi Greg,
I love your patch! Yet something to improve:
[auto build test ERROR on staging/staging-testing]
url:
https://github.com/0day-ci/linux/commits/Greg-Kroah-Hartman/staging-greybus-vibrator-use-proper-API-for-vibrator-devices/20210105-232001
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
1e9a9c7cba3ca5cbd3201a9f3b8dc6e8d7bef1c0
config: i386-randconfig-a013-20210105 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
#
https://github.com/0day-ci/linux/commit/4bfe9c5dc24fa35861d281448fd4091f652c2076
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Greg-Kroah-Hartman/staging-greybus-vibrator-use-proper-API-for-vibrator-devices/20210105-232001
git checkout 4bfe9c5dc24fa35861d281448fd4091f652c2076
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All errors (new ones prefixed by >>):
ld: drivers/staging/greybus/vibrator.o: in function `gb_vibrator_probe':
>> drivers/staging/greybus/vibrator.c:224: undefined reference to
>> `input_ff_create_memless'
vim +224 drivers/staging/greybus/vibrator.c
158
159 static int gb_vibrator_probe(struct gb_bundle *bundle,
160 const struct greybus_bundle_id *id)
161 {
162 struct greybus_descriptor_cport *cport_desc;
163 struct gb_connection *connection;
164 struct gb_vibrator_device *vib;
165 struct device *dev;
166 int retval;
167
168 if (bundle->num_cports != 1)
169 return -ENODEV;
170
171 cport_desc = &bundle->cport_desc[0];
172 if (cport_desc->protocol_id != GREYBUS_PROTOCOL_VIBRATOR)
173 return -ENODEV;
174
175 vib = kzalloc(sizeof(*vib), GFP_KERNEL);
176 if (!vib)
177 return -ENOMEM;
178
179 connection = gb_connection_create(bundle,
le16_to_cpu(cport_desc->id),
180NULL);
181 if (IS_ERR(connection)) {
182 retval = PTR_ERR(connection);
183 goto err_free_vib;
184 }
185 gb_connection_set_data(connection, vib);
186
187 vib->connection = connection;
188
189 greybus_set_drvdata(bundle, vib);
190
191 retval = gb_connection_enable(connection);
192 if (retval)
193 goto err_connection_destroy;
194
195 /*
196 * For now we create a device in sysfs for the vibrator, but
odds are
197 * there is a "real" device somewhere in the kernel for this,
but I
198 * can't find it at the moment...
199 */
200 vib->minor = ida_simple_get(&minors, 0, 0, GFP_KERNEL);
201 if (vib->minor < 0) {
202 retval = vib->minor;
203 goto err_connection_disable;
204 }
205 dev = device_create(&vibrator_class, &bundle->dev,
206 MKDEV(0, 0), vib, "vibrator%d", vib->minor);
207 if (IS_ERR(dev)) {
208 retval = -EINVAL;
209 goto err_ida_remove;
210 }
211 vib->dev = dev;
212
213 INIT_DELAYED_WORK(&vib->delayed_work, gb_vibrator_worker);
214
215 INIT_WORK(&vib->play_work, gb_vibrator_play_work);
216 vib->input->name = "greybus-vibrator";
217 vib->input->close = gb_vibrator_close;
218 vib->input->dev.parent = &bundle->dev;
219 vib->input->id.bustype = BUS_HOST;
220
221 input_set_drvdata(vib->input, vib);
222 input_set_capability(vib->input, EV_FF, FF_RUMBLE);
223
> 224 retval = input_ff_create_memless(vib->input, NULL,
225 gb_vibrator_play_effect);
226 if (retval)
227 goto err_device_remove;
228
229 gb_pm_runtime_put_autosuspend(bundle);
230
231 return 0;
232
233 err_device_remove:
234 device_unregister(vib->dev);
235 err_ida_remove:
236 ida_simple_remove(&minors, vib->minor);
237 err_connection_disable:
238 gb_connection_disable(connection);
239 err_connection_destroy:
240 gb_connection_destroy(connection);
241 err_free_vib:
242 kfree(vib);
243
244 return r
[PATCH] media: atomisp: ov2722: replace hardcoded function name
There is a debug message using hardcoded function name instead of the __func__ macro. Replace it. Report from checkpatch.pl on the file: WARNING: Prefer using '"%s...", __func__' to using 'ov2722_remove', this function's name, in a string + dev_dbg(&client->dev, "ov2722_remove...\n"); Signed-off-by: Filip Kolev --- drivers/staging/media/atomisp/i2c/atomisp-ov2722.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c index eecefcd734d0e..21d6bc62d452a 100644 --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c @@ -1175,7 +1175,7 @@ static int ov2722_remove(struct i2c_client *client) struct v4l2_subdev *sd = i2c_get_clientdata(client); struct ov2722_device *dev = to_ov2722_sensor(sd); - dev_dbg(&client->dev, "ov2722_remove...\n"); + dev_dbg(&client->dev, "%s...\n", __func__); dev->platform_data->csi_cfg(sd, 0); v4l2_ctrl_handler_free(&dev->ctrl_handler); -- 2.30.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: mt7621-dts: match pinctrl nodes with its binding documentation
On Mon, Jan 4, 2021 at 4:06 PM Sergio Paracuellos wrote: > According to the binding documentation pinctrl related nodes > must use '-pins$' and ''^(.*-)?pinmux$'' as names. Change all > to properly match them. Also default state is for consumer > nodes and shall be removed from here. > > Signed-off-by: Sergio Paracuellos Acked-by: Linus Walleij Yours, Linus Walleij ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] media: atomisp: ov2722: replace hardcoded function name
On Tue, Jan 05, 2021 at 10:29:18PM +0200, Filip Kolev wrote: > There is a debug message using hardcoded function name instead of the > __func__ macro. Replace it. > > Report from checkpatch.pl on the file: > > WARNING: Prefer using '"%s...", __func__' to using 'ov2722_remove', this > function's name, in a string > + dev_dbg(&client->dev, "ov2722_remove...\n"); > > Signed-off-by: Filip Kolev > --- > drivers/staging/media/atomisp/i2c/atomisp-ov2722.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c > b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c > index eecefcd734d0e..21d6bc62d452a 100644 > --- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c > +++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c > @@ -1175,7 +1175,7 @@ static int ov2722_remove(struct i2c_client *client) > struct v4l2_subdev *sd = i2c_get_clientdata(client); > struct ov2722_device *dev = to_ov2722_sensor(sd); > > - dev_dbg(&client->dev, "ov2722_remove...\n"); > + dev_dbg(&client->dev, "%s...\n", __func__); dev_dbg() provides the function name already, and this is just a "trace" call, and ftrace should be used instead, so the whole line should be removed entirely. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
