linux-kernel - Re: [PATCH v5 07/11] intel

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:   Thu, 23 Nov 2017 12:25:54 +0200
From:   Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     platform-driver-x86@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 07/11] intel_sgx: ptrace() support

On Thu, Nov 16, 2017 at 10:28:42AM +0100, Thomas Gleixner wrote:
> On Mon, 13 Nov 2017, Jarkko Sakkinen wrote:
> 
> > Implemented VMA callbacks in order to ptrace() debug enclaves.
> 
> The amount of information in this changelog is really overwhelming.
> 
> Please explain WHY you need that and HOW its supposed to work.
> 
> > +static inline int sgx_vma_access_word(struct sgx_encl *encl,
> > +				      unsigned long addr,
> > +				      void *buf,
> > +				      int len,
> > +				      int write,
> > +				      struct sgx_encl_page *encl_page,
> > +				      int i)
> 
> Can you find a way to waste more lines for a function declaration?
> 
> Aside of that using 'i' as a argument is just broken. Arguments should be
> self explaining as far as possible and sure not using names which are
> commonly used in code for iterators etc.
> 
> > +{
> > +	char data[sizeof(unsigned long)];
> > +	int align, cnt, offset;
> > +	void *vaddr;
> > +	int ret;
> > +
> > +	offset = ((addr + i) & (PAGE_SIZE - 1)) & ~(sizeof(unsigned long) - 1);
> > +	align = (addr + i) & (sizeof(unsigned long) - 1);
> 
> The kernel has macros for this kind of operations.
> 
> > +	cnt = sizeof(unsigned long) - align;
> > +	cnt = min(cnt, len - i);
> > +
> > +	if (write) {
> > +		if (encl_page->flags & SGX_ENCL_PAGE_TCS &&
> > +		    (offset < 8 || (offset + (len - i)) > 16))
> 
> Hard coded numbers which are nowhere explained are a nono. Please use proper
> defines and explain the meaning so the code becomes understandable.
> 
> > +			return -ECANCELED;
> > +
> > +		if (align || (cnt != sizeof(unsigned long))) {
> 
> What the heck is this doing? The complete lack of any comment in this
> code makes review impossible.
> 
> > +			vaddr = sgx_get_page(encl_page->epc_page);
> > +			ret = __edbgrd((void *)((unsigned long)vaddr + offset),
> > +				       (unsigned long *)data);
> 
> This typecast mess all over the place is just wrong. You either use the
> wrong variable types or your functions have the wrong parameter type.

Thank you for the feedback. I agree with all your comments.

I'll also split the function into sgx_vma_read_word() and
sgx_vma_write_word(). Makes the code somewhat cleaner and nicer to trace.

I have to admit that I have overlooked this commit when preparing the
patch set. Even though I've written code myself, it's been a while and
I had hard time to get a grip what is going on when I read your
response, which makes your feedback even more valid :-)

/Jarkko