linux-kernel - Re: Fwd: [PATCH v7 1/2] x86: Add Isolated Memory Regions for Quark X1000

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:	Fri, 30 Jan 2015 16:04:09 +0200
From:	Andy Shevchenko <andy.shevchenko@...il.com>
To:	"Bryan O'Donoghue" <pure.logic@...us-software.ie>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	"x86@...nel.org" <x86@...nel.org>,
	"dvhart@...radead.org" <dvhart@...radead.org>,
	"Ong, Boon Leong" <boon.leong.ong@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: Fwd: [PATCH v7 1/2] x86: Add Isolated Memory Regions for Quark X1000

> On 30/01/15 12:03, Andy Shevchenko wrote:

>> When CONFIG_DEBUGFS=n, you will get error pointer here, which is not NULL.
>>
>> So, the proper check is
>> if (IS_ERR())
>>   return PTR_ERR();
>> if (!file)
>>   return -ENOMEM;
>
> Yeah I saw that. Also saw that most other code doesn't bother trapping
> those return values - so skipped it.
> No issue adding.

Ok.

>>> +               } else {
>>> +                       reg = i;
>>
>> Do we go always through all IMRs and choose the last one?
>> If no, break is missed here.
>
> Yep - we always choose the last one.

OK.

>>> +       ret = imr_check_params(base, size);
>>> +       if (ret == -EINVAL || (ret == -ENOMEM && reg == -1))
>>
>> reg base size (0 correct, 1 wrong):
>>
>> 0 0 0 â€” which should be used? what is the priority?
>> 0 x 1 â€” index
>> 0 1 x â€” index
>> 1 0 0 â€” address
>> 1 0 1 â€” an error
>> 1 1 1 â€” an error
>>
>> Thus, could it be simpler? Like
>> if (reg < 0 && ret) ?
>
> ret will be EINVAL for unaligned base or size
> ret will be ENOMEM when reg == -1 and size == 0

ENOMEM only when size == 0.

I have read the function description. From my point of view there is
no difference what is in (base, size) if we have index set.

> I could probably write it like this to make it clearer
> (ret == -EINVAL || (reg == -1 && size == 0)
>         return -EINVAL;
>
> traps unaligned input - for address range tear-down
> traps zero sized - for address range tear-down
>
> Allows index based teardown i.e. reg >= 0

Again, seems a logic mistake here, or description of function is not correct.
Do we care what (base, size) if index is set? If so, why is it not
mentioned properly in the description?

Cite youself "If you specify an index on its own the base and size
parameters are ignored."
It's clearly fits to what I propose. Or I missed something?

>>> +               pr_warn("debugfs register failed!\n");
>>
>> Do we actually need this? Or move it to debug level.
>
> It was your suggestion @ a previous review ....

Yes, I suggested that is not a fatal error. Since we don't check (as
above) for CONFIG_DEBUGFS=n, we get the warning only if failed to
create a file. So, probably it's fine to leave like this.

-- 
With Best Regards,
Andy Shevchenko
--
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/