Prev: [PATCH v2 0/6] CFS Bandwidth Control
Next: Where to submit bug report about linux-next?
From: Oleg Nesterov on 28 Apr 2010 07:30
On 04/27, Serge E. Hallyn wrote:
>
> This introduces a Plan 9 style setuid capability filesystem.
> See Documentation/p9auth.txt for a description of how to use this.

Can't comment these changes due to the lack of knowledge, just
a couple of minor nits.

> +static ssize_t p9auth_use_write(struct file *file, const char __user *buffer,
> + size_t count, loff_t *ppos)
> +{
> + ssize_t retval = -ENOMEM;
> + char *user_buf;
> +
> + if (mutex_lock_interruptible(&cap_mutex))
> + return -EINTR;

EINTR doesn't look exactly right here, especially if TIF_SIGPENDING is
spurious. Probably ERESTARTNOINTR makes more sense. Or mutex_lock_killable().

> + user_buf = kzalloc(count+1, GFP_KERNEL);

Probably this is OK, but it looks a bit strange we do no check that
count is not too large.

> + if (copy_from_user(user_buf, buffer, count)) {
> + retval = -EFAULT;
> + goto out;
> + }
> +
> + retval = use_setuid_capability(user_buf);

It seems that use_setuid_capability() pathes assume that user_buf is
null terminated? Say, parse_user_capability() does kstrdup(user_buf).

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Serge E. Hallyn on 28 Apr 2010 11:20
Quoting Oleg Nesterov (oleg(a)redhat.com):
> On 04/27, Serge E. Hallyn wrote:
> >
> > This introduces a Plan 9 style setuid capability filesystem.
> > See Documentation/p9auth.txt for a description of how to use this.
>
> Can't comment these changes due to the lack of knowledge, just
> a couple of minor nits.

Thanks, Oleg.

> > +static ssize_t p9auth_use_write(struct file *file, const char __user *buffer,
> > + size_t count, loff_t *ppos)
> > +{
> > + ssize_t retval = -ENOMEM;
> > + char *user_buf;
> > +
> > + if (mutex_lock_interruptible(&cap_mutex))
> > + return -EINTR;
>
> EINTR doesn't look exactly right here, especially if TIF_SIGPENDING is
> spurious. Probably ERESTARTNOINTR makes more sense. Or mutex_lock_killable().

Ashwin had had this as ERESTARTSYS I believe. I'd read something about
userspace should only see -EINTR so I changed it. Sounds like I need
to follow the caller chain some more and learn a thing or two, before
I repost.

> > + user_buf = kzalloc(count+1, GFP_KERNEL);
>
> Probably this is OK, but it looks a bit strange we do no check that
> count is not too large.

Yes, I should check that, thanks!

> > + if (copy_from_user(user_buf, buffer, count)) {
> > + retval = -EFAULT;
> > + goto out;
> > + }
> > +
> > + retval = use_setuid_capability(user_buf);
>
> It seems that use_setuid_capability() pathes assume that user_buf is
> null terminated? Say, parse_user_capability() does kstrdup(user_buf).

I kzalloc()d to count+1 before, and only copy_from_user() count bytes,
so the last byte should always be 0.

Thanks again,

-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Oleg Nesterov on 28 Apr 2010 11:50
On 04/28, Serge E. Hallyn wrote:
>
> Quoting Oleg Nesterov (oleg(a)redhat.com):
>
> > > +static ssize_t p9auth_use_write(struct file *file, const char __user *buffer,
> > > + size_t count, loff_t *ppos)
> > > +{
> > > + ssize_t retval = -ENOMEM;
> > > + char *user_buf;
> > > +
> > > + if (mutex_lock_interruptible(&cap_mutex))
> > > + return -EINTR;
> >
> > EINTR doesn't look exactly right here, especially if TIF_SIGPENDING is
> > spurious. Probably ERESTARTNOINTR makes more sense. Or mutex_lock_killable().
>
> Ashwin had had this as ERESTARTSYS I believe. I'd read something about
> userspace should only see -EINTR so I changed it.

Yes, ERESTARTxxx should not be visible to the user-space. But it is OK
to return it from the syscall if signal_pending() is true, in this case
it will be changed to EINTR or the system call will be restarted.

ERESTARTNOINTR always restarts the syscall, perphaps after handling the
signal if it is really pending.

> > > + if (copy_from_user(user_buf, buffer, count)) {
> > > + retval = -EFAULT;
> > > + goto out;
> > > + }
> > > +
> > > + retval = use_setuid_capability(user_buf);
> >
> > It seems that use_setuid_capability() pathes assume that user_buf is
> > null terminated? Say, parse_user_capability() does kstrdup(user_buf).
>
> I kzalloc()d to count+1 before, and only copy_from_user() count bytes,
> so the last byte should always be 0.

Ah, indeed, thanks.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Serge E. Hallyn on 3 May 2010 20:00
Thanks for the comments, Oleg. I will fold this patch into the
main patch, but I'll spare everyone a full repost right now.

thanks,
-serge

From: Serge E. Hallyn <serue(a)us.ibm.com>
Subject: [PATCH 1/1] p9auth: address feedback

Address two comments by Oleg:

Return -ERESTARTNOINTR rather than -EINTR if we are interrupted
while waiting on cap_mutex. And check size of input buffer for
something sane. A user with hundreds of auxilliary groups could
end up with a > PAGE_SIZE message, so maybe this is a bit too
stringent, but I'm not sure how realistic that is.

Signed-off-by: Serge E. Hallyn <serue(a)us.ibm.com>
---
kernel/p9auth.c | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/kernel/p9auth.c b/kernel/p9auth.c
index a174373..5f89b4d 100644
--- a/kernel/p9auth.c
+++ b/kernel/p9auth.c
@@ -354,7 +354,10 @@ static ssize_t p9auth_grant_write(struct file *file, const char __user *buffer,
char *user_buf;

if (mutex_lock_interruptible(&cap_mutex))
- return -EINTR;
+ return -ERESTARTNOINTR;
+
+ if (count > PAGE_SIZE)
+ return -E2BIG;

user_buf = kzalloc(count+1, GFP_KERNEL);
if (!user_buf)
@@ -387,7 +390,10 @@ static ssize_t p9auth_use_write(struct file *file, const char __user *buffer,
char *user_buf;

if (mutex_lock_interruptible(&cap_mutex))
- return -EINTR;
+ return -ERESTARTNOINTR;
+
+ if (count > PAGE_SIZE)
+ return -E2BIG;

user_buf = kzalloc(count+1, GFP_KERNEL);
if (!user_buf)
--
1.7.0.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Howells on 4 May 2010 11:00
Serge E. Hallyn <serue(a)us.ibm.com> wrote:

> + result = kzalloc(MAX_DIGEST_SIZE, GFP_KERNEL);
> ...
> + ret = crypto_hash_digest(&desc, &sg, plain_text_size, result);

Does result have to be pre-cleared?

> + user_buf = kzalloc(count+1, GFP_KERNEL);
> + if (!user_buf)
> + goto out;
> +
> + if (copy_from_user(user_buf, buffer, count)) {

user_buf doesn't need preclearing. It's just a waste of time. This occurs
three times.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: [PATCH v2 0/6] CFS Bandwidth Control
Next: Where to submit bug report about linux-next?