linux-kernel - Re: [RFC PATCH 1/3] x86: Add another set of MSR accessor functions

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] [day] [month] [year] [list]

Date:	Mon, 17 Feb 2014 21:08:13 +0100
From:	Borislav Petkov <bp@...en8.de>
To:	"H. Peter Anvin" <hpa@...or.com>
Cc:	X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>,
	Borislav Petkov <bp@...e.de>
Subject: Re: [RFC PATCH 1/3] x86: Add another set of MSR accessor functions

On Mon, Feb 17, 2014 at 08:21:47AM -0800, H. Peter Anvin wrote:
> Good patch series overall, but I do have some issues with this one:
> 
> On 02/09/2014 05:48 AM, Borislav Petkov wrote:
> > + */
> > +int msr_read(u32 msr, struct msr *m)
> > +{
> > +	int err;
> > +	u64 val;
> > +
> > +	val = native_read_msr_safe(msr, &err);
> 
> I don't think we should use the native_ function here.

Ah, right, pv gunk, I conveniently victimized those cache lines away
from my head. :-P

rdmsrl_safe it is.

> 
> > +	if (err)
> > +		pr_warn("%s: Error reading MSR 0x%08x\n", __func__, msr);
> > +	else
> > +		m->q = val;
> 
> I also don't think we should print a message if the MSR doesn't exist.
> This will be a normal occurrence in a number of flows.

Right, I was suspecting the screaming in dmesg could upset people but
wasn't sure. Good point.

> > +static int __flip_bit(u32 msr, u8 bit, bool set)
> > +{
> > +	struct msr m;
> > +
> > +	if (bit > 63)
> > +		return -1;
> 
> Feels a bit excessive, but I'd suggest returning -EINVAL instead.

Ok.

> I would suggest explicitly making this an inline function.

Sure.

> > +	if (msr_read(msr, &m))
> > +		return -1;
> 
> Return -EIO?

Actually, msr_read already gives a retval so I can simply carry that
out. And it *is* -EIO already :-)

> How about:
> 
> 	m1 = m;
> 	if (set)
> 		m1.q |= BIT_64(bit);
> 	else
> 		m1.q &= ~BIT_64(bit);
> 
> 	if (m1.q != m.q) {
> 		if (msr_write(...))
> 			...

It's all the same to me, sure.

> Again, I'm not sure if printing a message here makes sense. In fact,
> this is the second message you print for the same thing.

Ok, I'll make them completely silent - if their users wanna say
something, they can do that based on the retval. Good.

Thanks for checking them out - I'll start playing with the revised
versions on real hw.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--
--
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/