[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <18886.9364.699476.946164@cargo.ozlabs.ibm.com>
Date: Sun, 22 Mar 2009 22:44:20 +1100
From: Paul Mackerras <paulus@...ba.org>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: Ingo Molnar <mingo@...e.hu>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] perfcounters: record time running and time enabled
for each counter
Andrew Morton writes:
> > > > - return put_user(cntval, (u64 __user *) buf) ? -EFAULT : sizeof(cntval);
> > > > + if (count != n * sizeof(u64))
> > > > + return -EINVAL;
> > > > +
> > > > + if (!access_ok(VERIFY_WRITE, buf, count))
> > > > + return -EFAULT;
> > > > +
> > >
> > > <panics>
> > >
> > > Oh.
> > >
> > > It would be a lot more reassuring to verify `uptr', rather than `buf' here.
>
> This?
>
> > > The patch adds new trailing whitespace. checkpatch helps.
> > >
> > > > + for (i = 0; i < n; ++i)
> > > > + if (__put_user(values[i], uptr + i))
> > > > + return -EFAULT;
> > >
> > > And here we iterate across `n', whereas we verified `count'.
> >
> > And the fact that we just verified count == n * 8, four lines above,
> > doesn't give you any comfort?
>
> access_ok(..., uptr, n * sizeof(*uptr))
>
> might be most robust.
I'm not sure quite what you're getting at. Are you saying
(a) you think there is an actual security problem in that code
(b) you can't tell if there is a problem in that code, or
(c) you think there isn't a problem but you had to spend too much
brain power working that out, or
(d) you think there isn't a problem but there might be in future if
someone changed the code?
> Or fix up the types (if needed) and copy the whole thing with copy_to_user()
>
> Is it really so performance-sensitive that we can't use plain old put_user()?
The sort of people that use performance-monitoring tools tend to be
sensitive to performance issues in their tools, for some reason. :)
Is copy_to_user on 8 bytes practically as fast as put_user, these
days?
Paul.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists