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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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/