Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1282991 > unrolled thread

Re: [RESEND v4 2/6] remoteproc: debugfs: Add ability to boot remote processor using debugfs

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RESEND v4 2/6] remoteproc: debugfs: Add ability to boot remote  processor using debugfs Lee Jones <lee.jones@linaro.org> - 2015-12-03 13:30 +0100
    Re: [RESEND v4 2/6] remoteproc: debugfs: Add ability to boot remote processor using debugfs Arnd Bergmann <arnd@arndb.de> - 2015-12-03 13:50 +0100
      Re: [RESEND v4 2/6] remoteproc: debugfs: Add ability to boot remote  processor using debugfs Lee Jones <lee.jones@linaro.org> - 2015-12-03 14:10 +0100
        Re: [RESEND v4 2/6] remoteproc: debugfs: Add ability to boot remote processor using debugfs Arnd Bergmann <arnd@arndb.de> - 2015-12-03 14:30 +0100
          Re: [RESEND v4 2/6] remoteproc: debugfs: Add ability to boot remote  processor using debugfs Lee Jones <lee.jones@linaro.org> - 2015-12-03 18:30 +0100
            Re: [RESEND v4 2/6] remoteproc: debugfs: Add ability to boot remote processor using debugfs Arnd Bergmann <arnd@arndb.de> - 2015-12-03 22:20 +0100
              Re: [RESEND v4 2/6] remoteproc: debugfs: Add ability to boot remote  processor using debugfs Bjorn Andersson <bjorn@kryo.se> - 2015-12-03 22:30 +0100
              Re: [RESEND v4 2/6] remoteproc: debugfs: Add ability to boot remote  processor using debugfs Lee Jones <lee.jones@linaro.org> - 2015-12-04 09:30 +0100

#1282991 — Re: [RESEND v4 2/6] remoteproc: debugfs: Add ability to boot remote processor using debugfs

On Fri, 27 Nov 2015, Bjorn Andersson wrote:

> On Tue, Nov 24, 2015 at 5:14 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > This functionality is especially useful during the testing phase.  When
> > used in conjunction with Mailbox's Test Framework we can trivially conduct
> > end-to-end testing i.e. boot co-processor, send and receive messages to
> > the co-processor, then shut it down again (repeat as required).
> >
> 
> I want this too!
> 
> > Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/remoteproc/remoteproc_debugfs.c | 34 +++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> > index 9d30809..8113c18 100644
> > --- a/drivers/remoteproc/remoteproc_debugfs.c
> > +++ b/drivers/remoteproc/remoteproc_debugfs.c
> > @@ -88,8 +88,42 @@ static ssize_t rproc_state_read(struct file *filp, char __user *userbuf,
> >         return simple_read_from_buffer(userbuf, count, ppos, buf, i);
> >  }
> >
> > +static ssize_t rproc_state_write(struct file *filp, const char __user *userbuf,
> > +                                size_t count, loff_t *ppos)
> > +{
> > +       struct rproc *rproc = filp->private_data;
> > +       char buf[10];
> > +       int ret;
> > +
> > +       if (count > sizeof(buf))
> > +               return count;
> > +
> > +       ret = copy_from_user(buf, userbuf, count);
> > +       if (ret)
> > +               return -EFAULT;
> > +
> > +       if (buf[count - 1] == '\n')
> > +               buf[count - 1] = '\0';
> 
> I believe you can get here with count = 0.

I'm pretty sure you can't.

If you are sure that you can, if you can provide me with a way of
testing, I'd be happy to put in provisions.

> > +
> > +       if (!strncmp(buf, "start", count)) {
> > +               ret = rproc_boot(rproc);
> > +               if (ret) {
> > +                       dev_err(&rproc->dev, "Boot failed: %d\n", ret);
> > +                       return ret;
> > +               }
> > +       } else if (!strncmp(buf, "stop", count)) {
> > +               rproc_shutdown(rproc);
> > +       } else {
> > +               dev_err(&rproc->dev, "Unrecognised option: %s\n", buf);
> 
> Unrecognized

What I have is correct.

> > +               return -EINVAL;
> > +       }
> > +
> > +       return count;
> > +}
> > +
> >  static const struct file_operations rproc_state_ops = {
> >         .read = rproc_state_read,
> > +       .write = rproc_state_write,
> >         .open = simple_open,
> >         .llseek = generic_file_llseek,
> >  };
> 
> Part of these nits
> 
> Acked-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>

Thanks.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1283021 — Re: [RESEND v4 2/6] remoteproc: debugfs: Add ability to boot remote processor using debugfs

On Thursday 03 December 2015 12:26:34 Lee Jones wrote:
> > >
> > > +static ssize_t rproc_state_write(struct file *filp, const char __user *userbuf,
> > > +                                size_t count, loff_t *ppos)
> > > +{
> > > +       struct rproc *rproc = filp->private_data;
> > > +       char buf[10];
> > > +       int ret;
> > > +
> > > +       if (count > sizeof(buf))
> > > +               return count;
> > > +       ret = copy_from_user(buf, userbuf, count);
> > > +       if (ret)
> > > +               return -EFAULT;
> > > +
> > > +       if (buf[count - 1] == '\n')
> > > +               buf[count - 1] = '\0';
> > 
> > I believe you can get here with count = 0.
> 
> I'm pretty sure you can't.
> 
> If you are sure that you can, if you can provide me with a way of
> testing, I'd be happy to put in provisions.
> 

I think that a zero-length write() from user space ends up in the write
file operation.

Also, we get a gcc warning about the out-of-bounds access for code like
this, and checking that count is larger than zero avoids the warning.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283031

On Thu, 03 Dec 2015, Arnd Bergmann wrote:

> On Thursday 03 December 2015 12:26:34 Lee Jones wrote:
> > > >
> > > > +static ssize_t rproc_state_write(struct file *filp, const char __user *userbuf,
> > > > +                                size_t count, loff_t *ppos)
> > > > +{
> > > > +       struct rproc *rproc = filp->private_data;
> > > > +       char buf[10];
> > > > +       int ret;
> > > > +
> > > > +       if (count > sizeof(buf))
> > > > +               return count;
> > > > +       ret = copy_from_user(buf, userbuf, count);
> > > > +       if (ret)
> > > > +               return -EFAULT;
> > > > +
> > > > +       if (buf[count - 1] == '\n')
> > > > +               buf[count - 1] = '\0';
> > > 
> > > I believe you can get here with count = 0.
> > 
> > I'm pretty sure you can't.
> > 
> > If you are sure that you can, if you can provide me with a way of
> > testing, I'd be happy to put in provisions.
> > 
> 
> I think that a zero-length write() from user space ends up in the write
> file operation.

I tested this and didn't see it enter write().  My conclusion was that
if the file doesn't change, then nothing is triggered.

> Also, we get a gcc warning about the out-of-bounds access for code like
> this, and checking that count is larger than zero avoids the warning.

I did however see this warning and wondered what it was talking
about.  Thanks for clarifying.  Will fix.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283042 — Re: [RESEND v4 2/6] remoteproc: debugfs: Add ability to boot remote processor using debugfs

On Thursday 03 December 2015 13:03:41 Lee Jones wrote:
> On Thu, 03 Dec 2015, Arnd Bergmann wrote:
> 
> > On Thursday 03 December 2015 12:26:34 Lee Jones wrote:
> > > > >
> > > > > +static ssize_t rproc_state_write(struct file *filp, const char __user *userbuf,
> > > > > +                                size_t count, loff_t *ppos)
> > > > > +{
> > > > > +       struct rproc *rproc = filp->private_data;
> > > > > +       char buf[10];
> > > > > +       int ret;
> > > > > +
> > > > > +       if (count > sizeof(buf))
> > > > > +               return count;
> > > > > +       ret = copy_from_user(buf, userbuf, count);
> > > > > +       if (ret)
> > > > > +               return -EFAULT;
> > > > > +
> > > > > +       if (buf[count - 1] == '\n')
> > > > > +               buf[count - 1] = '\0';
> > > > 
> > > > I believe you can get here with count = 0.
> > > 
> > > I'm pretty sure you can't.
> > > 
> > > If you are sure that you can, if you can provide me with a way of
> > > testing, I'd be happy to put in provisions.
> > > 
> > 
> > I think that a zero-length write() from user space ends up in the write
> > file operation.
> 
> I tested this and didn't see it enter write().  My conclusion was that
> if the file doesn't change, then nothing is triggered.
> 

Ah, interesting. I haven't tried myself, and just tried to read the
code. Maybe glibc already catches zero-length writes before it gets
into the kernel, or I just missed the part of the syscall that checks
for this.

	arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283210

On Thu, 03 Dec 2015, Arnd Bergmann wrote:

> On Thursday 03 December 2015 13:03:41 Lee Jones wrote:
> > On Thu, 03 Dec 2015, Arnd Bergmann wrote:
> > 
> > > On Thursday 03 December 2015 12:26:34 Lee Jones wrote:
> > > > > >
> > > > > > +static ssize_t rproc_state_write(struct file *filp, const char __user *userbuf,
> > > > > > +                                size_t count, loff_t *ppos)
> > > > > > +{
> > > > > > +       struct rproc *rproc = filp->private_data;
> > > > > > +       char buf[10];
> > > > > > +       int ret;
> > > > > > +
> > > > > > +       if (count > sizeof(buf))
> > > > > > +               return count;
> > > > > > +       ret = copy_from_user(buf, userbuf, count);
> > > > > > +       if (ret)
> > > > > > +               return -EFAULT;
> > > > > > +
> > > > > > +       if (buf[count - 1] == '\n')
> > > > > > +               buf[count - 1] = '\0';
> > > > > 
> > > > > I believe you can get here with count = 0.
> > > > 
> > > > I'm pretty sure you can't.
> > > > 
> > > > If you are sure that you can, if you can provide me with a way of
> > > > testing, I'd be happy to put in provisions.
> > > > 
> > > 
> > > I think that a zero-length write() from user space ends up in the write
> > > file operation.
> > 
> > I tested this and didn't see it enter write().  My conclusion was that
> > if the file doesn't change, then nothing is triggered.
> > 
> 
> Ah, interesting. I haven't tried myself, and just tried to read the
> code. Maybe glibc already catches zero-length writes before it gets
> into the kernel, or I just missed the part of the syscall that checks
> for this.

Glibc is responsible indeed:
  
  http://osxr.org/glibc/source/io/write.c

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283391 — Re: [RESEND v4 2/6] remoteproc: debugfs: Add ability to boot remote processor using debugfs

On Thursday 03 December 2015 17:28:30 Lee Jones wrote:
> > 
> > Ah, interesting. I haven't tried myself, and just tried to read the
> > code. Maybe glibc already catches zero-length writes before it gets
> > into the kernel, or I just missed the part of the syscall that checks
> > for this.
> 
> Glibc is responsible indeed:
>   
>   http://osxr.org/glibc/source/io/write.c

Ok, so an attacker can force the stack overflow by calling
syscall(__NR_write, fd, p, 0) if that has any potential value,
but normal users won't hit this case.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283397

On Thu, Dec 3, 2015 at 1:12 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 03 December 2015 17:28:30 Lee Jones wrote:
>> >
>> > Ah, interesting. I haven't tried myself, and just tried to read the
>> > code. Maybe glibc already catches zero-length writes before it gets
>> > into the kernel, or I just missed the part of the syscall that checks
>> > for this.
>>
>> Glibc is responsible indeed:
>>
>>   http://osxr.org/glibc/source/io/write.c
>
> Ok, so an attacker can force the stack overflow by calling
> syscall(__NR_write, fd, p, 0) if that has any potential value,
> but normal users won't hit this case.
>

It seems glibc might be the only libc implementation with this protection.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1283629

On Thu, 03 Dec 2015, Arnd Bergmann wrote:

> On Thursday 03 December 2015 17:28:30 Lee Jones wrote:
> > > 
> > > Ah, interesting. I haven't tried myself, and just tried to read the
> > > code. Maybe glibc already catches zero-length writes before it gets
> > > into the kernel, or I just missed the part of the syscall that checks
> > > for this.
> > 
> > Glibc is responsible indeed:
> >   
> >   http://osxr.org/glibc/source/io/write.c
> 
> Ok, so an attacker can force the stack overflow by calling
> syscall(__NR_write, fd, p, 0) if that has any potential value,
> but normal users won't hit this case.

Right.  I have fixed the issue (and another one I found) anyway, if
only to rid the GCC warning.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web