From: Michael Zoran <[email protected]>
Signed-off-by: Michael Zoran <[email protected]>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 32d12e6..98c6819 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
@@ -219,7 +219,7 @@ remote_event_signal(REMOTE_EVENT_T *event)
int
vchiq_copy_from_user(void *dst, const void *src, int size)
{
- if ((uint32_t)src < TASK_SIZE) {
+ if ((unsigned long)src < TASK_SIZE) {
return copy_from_user(dst, src, size);
} else {
memcpy(dst, src, size);
--
2.9.3
On Sun, Oct 23, 2016 at 10:29:18PM -0700, [email protected] wrote:
> From: Michael Zoran <[email protected]>
>
> Signed-off-by: Michael Zoran <[email protected]>
> ---
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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 32d12e6..98c6819 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
> @@ -219,7 +219,7 @@ remote_event_signal(REMOTE_EVENT_T *event)
> int
> vchiq_copy_from_user(void *dst, const void *src, int size)
> {
> - if ((uint32_t)src < TASK_SIZE) {
> + if ((unsigned long)src < TASK_SIZE) {
> return copy_from_user(dst, src, size);
> } else {
> memcpy(dst, src, size);
This code looks totally wrong still. That's not how we differentiate
kernel pointers from user pointers.
Also no commit message (I tend not to read the subject) so it took me a
while to figure out what was going on. Anyway, everything needs a
commit message. That's a rule.
regards,
dan carpenter
On Mon, 2016-10-24 at 13:54 +0300, Dan Carpenter wrote:
> On Sun, Oct 23, 2016 at 10:29:18PM -0700, [email protected] wrote:
> > From: Michael Zoran <[email protected]>
> >
> > Signed-off-by: Michael Zoran <[email protected]>
> > ---
> > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
> > | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > 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 32d12e6..98c6819 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
> > @@ -219,7 +219,7 @@ remote_event_signal(REMOTE_EVENT_T *event)
> > int
> > vchiq_copy_from_user(void *dst, const void *src, int size)
> > {
> > - if ((uint32_t)src < TASK_SIZE) {
> > + if ((unsigned long)src < TASK_SIZE) {
> > return copy_from_user(dst, src, size);
> > } else {
> > memcpy(dst, src, size);
>
>
> This code looks totally wrong still. That's not how we differentiate
> kernel pointers from user pointers.
>
> Also no commit message (I tend not to read the subject) so it took me
> a
> while to figure out what was going on. Anyway, everything needs a
> commit message. That's a rule.
>
> regards,
> dan carpenter
>
I didn't think it looked totally correct, but I'm not sure it's any
more broken then what is already in the tree.
If you can kindly point me to some other source code or documentation
to look at that is correct, I'm more then willing to fix the patch.
On Mon, Oct 24, 2016 at 04:09:37AM -0700, Michael Zoran wrote:
> I didn't think it looked totally correct, but I'm not sure it's any
> more broken then what is already in the tree.
It's not more broken. But better to leave the compile warning there to
mark that it is an obvious security problem.
>
> If you can kindly point me to some other source code or documentation
> to look at that is correct, I'm more then willing to fix the patch.
>
I was hoping the maintainers could chip in, because I didn't want to
look at the code. We really need to track which are use pointers and
which are kernel pointers. We can't mix them like this.
regards,
dan carpenter
On Mon, 2016-10-24 at 14:36 +0300, Dan Carpenter wrote:
> On Mon, Oct 24, 2016 at 04:09:37AM -0700, Michael Zoran wrote:
> > I didn't think it looked totally correct, but I'm not sure it's any
> > more broken then what is already in the tree.
>
> It's not more broken. But better to leave the compile warning there
> to
> mark that it is an obvious security problem.
>
> >
> > If you can kindly point me to some other source code or
> > documentation
> > to look at that is correct, I'm more then willing to fix the patch.
> >
>
> I was hoping the maintainers could chip in, because I didn't want to
> look at the code. We really need to track which are use pointers and
> which are kernel pointers. We can't mix them like this.
>
> regards,
> dan carpenter
>
The problem is that I'm mostly interested in arm64 ATM, and I don't
think the existing code works at all with 64 bit pointers.
Broken as it may be...
On Mon, Oct 24, 2016 at 04:39:49AM -0700, Michael Zoran wrote:
> On Mon, 2016-10-24 at 14:36 +0300, Dan Carpenter wrote:
> > On Mon, Oct 24, 2016 at 04:09:37AM -0700, Michael Zoran wrote:
> > > I didn't think it looked totally correct, but I'm not sure it's any
> > > more broken then what is already in the tree.
> >
> > It's not more broken.??But better to leave the compile warning there
> > to
> > mark that it is an obvious security problem.
> >
> > >
> > > If you can kindly point me to some other source code or
> > > documentation
> > > to look at that is correct, I'm more then willing to fix the patch.
> > >
> >
> > I was hoping the maintainers could chip in, because I didn't want to
> > look at the code.??We really need to track which are use pointers and
> > which are kernel pointers.??We can't mix them like this.
> >
> > regards,
> > dan carpenter
> >
>
> The problem is that I'm mostly interested in arm64 ATM, and I don't
> think the existing code works at all with 64 bit pointers.
>
> Broken as it may be...
It's a security issue. We'll get this fixed in a day or two.
regards,
dan carpenter
On Mon, 2016-10-24 at 14:58 +0300, Dan Carpenter wrote:
> On Mon, Oct 24, 2016 at 04:39:49AM -0700, Michael Zoran wrote:
> > On Mon, 2016-10-24 at 14:36 +0300, Dan Carpenter wrote:
> > > On Mon, Oct 24, 2016 at 04:09:37AM -0700, Michael Zoran wrote:
> > > > I didn't think it looked totally correct, but I'm not sure it's
> > > > any
> > > > more broken then what is already in the tree.
> > >
> > > It's not more broken. But better to leave the compile warning
> > > there
> > > to
> > > mark that it is an obvious security problem.
> > >
> > > >
> > > > If you can kindly point me to some other source code or
> > > > documentation
> > > > to look at that is correct, I'm more then willing to fix the
> > > > patch.
> > > >
> > >
> > > I was hoping the maintainers could chip in, because I didn't want
> > > to
> > > look at the code. We really need to track which are use pointers
> > > and
> > > which are kernel pointers. We can't mix them like this.
> > >
> > > regards,
> > > dan carpenter
> > >
> >
> > The problem is that I'm mostly interested in arm64 ATM, and I don't
> > think the existing code works at all with 64 bit pointers.
> >
> > Broken as it may be...
>
> It's a security issue. We'll get this fixed in a day or two.
>
> regards,
> dan carpenter
If security is a major goal with this driver, I think the whole driver
needs to be thrown out the door and rewritten from scratch!
This driver is for the Raspberry PI and a very, very big assumption
that is in the whole architecture is that local processes are trusted.
I can give you probably a phone book of issues like this with this
driver, but I'm thinking that's outside the scope of this patch set and
outside the scope of what I'm trying to do.
I'm saying just be patient a bit. The devs are probably in CA and it's
still 5:30 AM there...
regards,
dan carpenter
On Sun, Oct 23, 2016 at 10:29:18PM -0700, [email protected] wrote:
> From: Michael Zoran <[email protected]>
>
> Signed-off-by: Michael Zoran <[email protected]>
> ---
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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 32d12e6..98c6819 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
> @@ -219,7 +219,7 @@ remote_event_signal(REMOTE_EVENT_T *event)
> int
> vchiq_copy_from_user(void *dst, const void *src, int size)
> {
> - if ((uint32_t)src < TASK_SIZE) {
> + if ((unsigned long)src < TASK_SIZE) {
> return copy_from_user(dst, src, size);
> } else {
> memcpy(dst, src, size);
Ick, that's horrid.
And I can't take patches without a changelog text.
Please fix up the callers to do the right thing, this shouldn't be a
wrapper function at all, especially given the mess of a cast as Dan
points out. Just call the correct copy_from_user() call and handle the
correct error return value.
thanks,
greg k-h
On Mon, 2016-10-24 at 15:23 +0200, Greg KH wrote:
> On Sun, Oct 23, 2016 at 10:29:18PM -0700, [email protected] wrote:
> > From: Michael Zoran <[email protected]>
> >
> > Signed-off-by: Michael Zoran <[email protected]>
> > ---
> > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
> > | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > 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 32d12e6..98c6819 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
> > @@ -219,7 +219,7 @@ remote_event_signal(REMOTE_EVENT_T *event)
> > int
> > vchiq_copy_from_user(void *dst, const void *src, int size)
> > {
> > - if ((uint32_t)src < TASK_SIZE) {
> > + if ((unsigned long)src < TASK_SIZE) {
> > return copy_from_user(dst, src, size);
> > } else {
> > memcpy(dst, src, size);
>
> Ick, that's horrid.
>
> And I can't take patches without a changelog text.
>
> Please fix up the callers to do the right thing, this shouldn't be a
> wrapper function at all, especially given the mess of a cast as Dan
> points out. Just call the correct copy_from_user() call and handle
> the
> correct error return value.
>
> thanks,
>
> greg k-h
OK, this is getting a wee bit outside the scope of checking in arm64
fixes. I was simply invited to submit my arm64 specific changes to the
stagging tree.
I'm starting to wonder if perhaps the "proper" maintainers should take
over this issue at this point. Perhaps some of the people that were
involved in checking this into the upstream tree to begin with.
I'm new to the whole kernel source code and we are getting into
architecture issues.
If you want to wait for someone else to take over that's fine. If you
want to send a patch that's also fine. There is not really strict
ownership of anything in the kernel. Greg will take patches from
anyone and he gets the last word wrt to drivers/staging/ (which sort of
contradicts what I said earlier about ownership).
regards,
dan carpenter
At this point, I need to take a step back from this whole thing and
evaluate where this is all going. I started this arm64 thing for fun
as a tinker idea, and it is quickly becoming more then I think I want
to get involved with at this time.
I'm not saying I won't return to this at a later time or tinker with it
on my own. I just need to take a step back. I have sent out patches
and if someone else wants to use what I have submitted as a starting
point, that's completely fine with me.
Thanks.
On Mon, 2016-10-24 at 17:46 +0300, Dan Carpenter wrote:
> If you want to wait for someone else to take over that's fine. If
> you
> want to send a patch that's also fine. There is not really strict
> ownership of anything in the kernel. Greg will take patches from
> anyone and he gets the last word wrt to drivers/staging/ (which sort
> of
> contradicts what I said earlier about ownership).
>
> regards,
> dan carpenter
>