Re: [PATCH v2] perfcounters: record time running and time enabled for each counter

[Paul Mackerras <paulus@samba.org>]

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.