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

[Paul Mackerras]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/