linux-kernel - Re: [PATCH v2 1/1] 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:	Sat, 24 Jan 2015 13:02:28 +0200
From:	Andy Shevchenko <andy.shevchenko@...il.com>
To:	"Ong, Boon Leong" <boon.leong.ong@...el.com>
Cc:	"Bryan O'Donoghue" <pure.logic@...us-software.ie>,
	"tglx@...utronix.de" <tglx@...utronix.de>,
	"mingo@...hat.com" <mingo@...hat.com>,
	"hpa@...or.com" <hpa@...or.com>, "x86@...nel.org" <x86@...nel.org>,
	"dvhart@...radead.org" <dvhart@...radead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/1] x86: Add Isolated Memory Regions for Quark X1000

On Sat, Jan 24, 2015 at 3:48 AM, Ong, Boon Leong
<boon.leong.ong@...el.com> wrote:

>>+static int imr_enabled(struct imr_regs *imr)
> Do we want to make it inline perhaps since it is 1 liner?

Since it is declared static I would even suggest the new name is_imr_enabled().

[]

>>+int imr_remove_range(int reg, unsigned long base, unsigned long size)
>>+{
>>+      struct imr_regs imr;
>>+      int found = 0, i, ret = 0;
> Please make each of the defined variables as individual line here..

I would suggest to type i as unsigned int and found as bool.

[]

>>+              if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK) {
>>+                      ret = -ENODEV;
>>+                      goto done;
>>+              }
>>+              found = 1;
>>+

Redundant empty line.

>>+      } else {
>>+              /* Search for match based on address range */
>>+              for (i = 0; i < imr_dev.max_imr; i++) {
>>+                      ret = imr_read(reg, &imr);
> A serious bug here.... 'reg' should be 'i' . We enter this branch if reg=-1
> Is there a miss in your test case?
>
>>+                      if (ret)
>>+                              goto done;
>>+
>>+                      if (!imr_enabled(&imr) || imr.addr_lo & IMR_LOCK)
>>+                              continue;
>>+
>>+                      if ((imr_to_phys(imr.addr_lo) == base) &&
>>+                          (imr_to_phys(imr.addr_hi) == max)) {
> I think we need to take care of the size that has been fix-up here ...
>
>>+                              found = 1;
>>+                              reg = i;

According to your comment this line becomes redundant.

[]

>>+static void __init imr_fixup_memmap(void)
>>+{
>>+      unsigned long base  = virt_to_phys(&_text);
>>+      unsigned long size = virt_to_phys(&__end_rodata) - base;
> What about the size fixup to be consistent?
> We should not guard more than it is needed .....
>
>>+      int i, ret;
> Two int declaration line here.

unsigned int i; ?

-- 
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/