access_ok macor

I found that I can setup text base in binutils/ld/emulparam/elf32mb_linux.sh

The problem which I have is that if I run socketpair, getsockname, getpeername LTP tests with invalid salen pointer there are addresses close to 0x0. Microblaze has no text there and the sigsegv fault is generated.

This fault could be fixed by changed access_ok macro where I check bottom limit at 0x1000 0000 too. After this change the LTP program not failed but I am not sure if is the right solution because none arch do it. All archs just check upper limit not lower.

What is the correct solution for it? Moving .text base to 0x0 or is there any other elegant solution?

Quoted 5 lines · Michal Simek

I found that I can setup text base in binutils/ld/emulparam/elf32mb_linux.sh

The problem which I have is that if I run socketpair, getsockname, getpeername LTP tests with invalid salen pointer there are addresses close to 0x0. Microblaze has no text there and the sigsegv fault is generated.

This sounds like a classic NULL pointer dereference that is handled correctly by the kernel. The question is where the address came from.

Quoted 8 lines

This fault could be fixed by changed access_ok macro where I check bottom limit at 0x1000 0000 too. After this change the LTP program not failed but I am not sure if is the right solution because none arch do it. All archs just check upper limit not lower.

What is the correct solution for it? Moving .text base to 0x0 or is there any other elegant solution?

Moving .text is not the right solution, because it only papers over real bugs.

access_ok() is also not the right place to check this, the only purpose it has is to make sure that the argument is not a valid kernel address but either a valid user address or possibly invalid address. Also, access_ok() is only used together with the copy_from/to_user and get/put_user function families. These need to catch invalid addresses with a fixup table entry in the kernel.

I briefly looked at your implementation but could not find any problems in this area. Could you use gdb to find out whether the sigsegv happens in the kernel at all, or in user space?

	Arnd <><

Arnd Bergmann wrote:
Look at
http://developer.petalogix.com/git/gitweb.cgi?p=ltp-microblaze.git;a=commitdiff;h=45f4cd783ce8b94f1267bb87c0c46e8536f62eca

There are three affected tests and my quick fixes which I am trying to solve now.

int move_addr_to_user(struct sockaddr *kaddr, int klen, void __user *uaddr,
int __user *ulen)

{
	int err;
	int len;

	err = get_user(len, ulen);
	if (err)
		return err;

Quoted 6 lines · Michal Simek

Arnd Bergmann wrote:
Look at
http://developer.petalogix.com/git/gitweb.cgi?p=ltp-microblaze.git;a=commitdiff;h=45f4cd783ce8b94f1267bb87c0c46e8536f62eca

There are three affected tests and my quick fixes which I am trying to solve now.

ok, I see.

Quoted 10 lines

int move_addr_to_user(struct sockaddr *kaddr, int klen, void __user *uaddr,
int __user *ulen)

{
	int err;
	int len;

	err = get_user(len, ulen);
	if (err)
		return err;

So the code looks something like

                        "1:     lw      %1, %2, r0;                     \
                                addk    %0, r0, r0;                     \
                        2:                                              \
                        .section .fixup,\"ax\";                         \
                        3:      brid    2b;                             \
                                addik   %0, r0, %3;                     \
                        .previous;                                      \
                        .section ,\"a\";                      		\
                        .word   1b,3b;                                  \
                        .previous;"                                     \

Not much that can go wrong there. First of all, I'd check that the code actually looks the same in the binary. I assume that the 'addik' gets executed when the brid branches, right?

I would guess that some of the logic in do_page_fault might be broken and does not actually call the fixup.

	Arnd <><

Quoted 7 lines · Arnd Bergmann

Quoted 4 lines

r29=00000000, r30=00000000, r31=CE9759A4, rPC=C000123C msr=800045AE, ear=00000001, esr=000000B2, fsr=000080D0
Segmentation fault

I guess then you should check if 0xc000123c is in your exception table, or why it is not.

on that address is load instruction for unaligned exception because addr is odd number that's why is called unaligned exception handler and from this function is called load instruction which failed. :-(

Currently this make more sense why that tests failed. If that pointers are even number exception is not taken and exception sure don't have fixup for it because this is generic code. :-(

That's the problem because we are looking for regs->pc but this point to unaligned exception handler.

Quoted 12 lines · Michal Simek

Quoted 6 lines · Arnd Bergmann

Quoted 4 lines

r29=00000000, r30=00000000, r31=CE9759A4, rPC=C000123C msr=800045AE, ear=00000001, esr=000000B2, fsr=000080D0
Segmentation fault

I guess then you should check if 0xc000123c is in your exception table, or why it is not.

on that address is load instruction for unaligned exception because addr is odd number that's why is called unaligned exception handler and from this function is called load instruction which failed. :-(

Quoted 6 lines

Currently this make more sense why that tests failed. If that pointers are even number exception is not taken and exception sure don't have fixup for it because this is generic code. :-(

That's the problem because we are looking for regs->pc but this point to unaligned exception handler.

Ok, that makes a lot of sense.

The solution then is to handle fixups from the unaligned exception handler if you come from the kernel. That should fix the three text cases.

I don't fully understand your exception handling there, but I think you also need to add code checking for __range_ok() to your unaligned handler, to prevent malicious user space code from accessing the kernel through unaligned pointers.

	Arnd <><

when the code tried to read/write from unaligned address (and in cpu is turn on unaligned exception) then is caused unaligned exception and asm code assemble/return value which is on that unaligned address. (Assemble it that read/write every byte separately). That will be harder to prevent all this cases because unaligned exception is in generic code.
What do you mean add __range_ok? Range checking is ok. The problem is when in case get_user kernel try to load unaligned addr - unaligned exception is perform and try to load that value separately. If that page is not there, page fault handler is called and not find it, it is performed search from exception table and that address is not there of course - because address in pc is generic unaligned code. I think that handling this needs more code.
Maybe if the address with from unaligned exception handler (there are some address which can caused it) and find out which aligned address is there and find out proper fixup for it.
I think that this could work.

What do you think?

Quoted 13 lines · Michal Simek

when the code tried to read/write from unaligned address (and in cpu is turn on unaligned exception) then is caused unaligned exception and asm code assemble/return value which is on that unaligned address. (Assemble it that read/write every byte separately). That will be harder to prevent all this cases because unaligned exception is in generic code.
What do you mean add __range_ok? Range checking is ok. The problem is when in case get_user kernel try to load unaligned addr - unaligned exception is perform and try to load that value separately. If that page is not there, page fault handler is called and not find it, it is performed search from exception table and that address is not there of course - because address in pc is generic unaligned code. I think that handling this needs more code.
Maybe if the address with from unaligned exception handler (there are some address which can caused it) and find out which aligned address is there and find out proper fixup for it.
I think that this could work.

Quoted 1 line

What do you think?

I think the key point is that the kernel should never try an unaligned access. Other architectures already rely on this, so you can too.

Instead of doing an aligned access from the unaligned exception handler, you should not access the data at all when you come from kernel mode but only search the fixup table to see if you should continue anyway.

Sorry for adding confusion by mentioning the __range_ok, which is addressing a separate problem. I should have made that clearer.

In pseudo-code, I think you should do:

void unaligned_exception(struct pt_regs *regs, unsigned long address)

{
	if (kernel_mode(regs)) {
		/*
		 * unaligned access not allowed in kernel,
		 * but search exception table.
		 */
		bad_page_fault(regs, address, 0);
	} else {
		if (!access_ok(address, 4))
			force_sig(SIGBUS, current);
		else
			fixup_unaligned_access(regs);
	}
}

	Arnd <><

Quoted 17 lines · Michal Simek

when the code tried to read/write from unaligned address (and in cpu is turn on unaligned exception) then is caused unaligned exception and asm code assemble/return value which is on that unaligned address. (Assemble it that read/write every byte separately). That will be harder to prevent all this cases because unaligned exception is in generic code. What do you mean add __range_ok? Range checking is ok. The problem is when in case get_user kernel try to load unaligned addr - unaligned exception is perform and try to load that value separately. If that page is not there, page fault handler is called and not find it, it is performed search from exception table and that address is not there of course - because address in pc is generic unaligned code. I think that handling this needs more code. Maybe if the address with from unaligned exception handler (there are some address which can caused it) and find out which aligned address is there and find out proper fixup for it. I think that this could work.

Quoted 1 line

What do you think?

I think the key point is that the kernel should never try an unaligned access. Other architectures already rely on this, so you can too.

Quoted 18 lines · Arnd Bergmann

Quoted 17 lines · Michal Simek

when the code tried to read/write from unaligned address (and in cpu is turn on unaligned exception) then is caused unaligned exception and asm code assemble/return value which is on that unaligned address. (Assemble it that read/write every byte separately). That will be harder to prevent all this cases because unaligned exception is in generic code. What do you mean add __range_ok? Range checking is ok. The problem is when in case get_user kernel try to load unaligned addr - unaligned exception is perform and try to load that value separately. If that page is not there, page fault handler is called and not find it, it is performed search from exception table and that address is not there of course - because address in pc is generic unaligned code. I think that handling this needs more code. Maybe if the address with from unaligned exception handler (there are some address which can caused it) and find out which aligned address is there and find out proper fixup for it. I think that this could work.

Quoted 3 lines

Quoted 1 line

What do you think?

I think the key point is that the kernel should never try an unaligned access. Other architectures already rely on this, so you can too.

No, other architectures used to rely on this, until it was no longer possible to do so. See for example, nfs. Unaligned accesses by the kernel must be handled by the architecture, unaligned accesses by userspace can be optionally fixed up.

Quoted 11 lines · Michal Simek

Quoted 7 lines · Arnd Bergmann

Quoted 4 lines

r29=00000000, r30=00000000, r31=CE9759A4, rPC=C000123C msr=800045AE, ear=00000001, esr=000000B2, fsr=000080D0
Segmentation fault

I guess then you should check if 0xc000123c is in your exception table, or why it is not.

on that address is load instruction for unaligned exception because addr

is odd number

Quoted 4 lines

that's why is called unaligned exception handler and from this function is called load instruction which failed. :-(

Currently this make more sense why that tests failed. If that pointers

are

Quoted 1 line

even number exception is not taken and exception sure don't have fixup

for it because

Quoted 3 lines

this is generic code. :-(

That's the problem because we are looking for regs->pc but this point to

unaligned exception

Quoted 1 line

handler.

Ok, that makes a lot of sense.

The solution then is to handle fixups from the unaligned exception handler if you come from the kernel. That should fix the three text cases.

I don't fully understand your exception handling there, but I think you also need to add code checking for __range_ok() to your unaligned handler, to prevent malicious user space code from accessing the kernel through unaligned pointers.

Quoted 3 lines · Arnd Bergmann

The solution then is to handle fixups from the unaligned exception handler if you come from the kernel. That should fix the three text cases.

Quoted 4 lines

I don't fully understand your exception handling there, but I think you also need to add code checking for __range_ok() to your unaligned handler, to prevent malicious user space code from accessing the kernel through unaligned pointers.

Just to try to clarify - are there any alignment rules in the ABI on user-space pointers (which end up going to get/put_user)?

It seems the failure path is like this:

1. userspace passes unaligned pointer
2. get_user attempts to access
3. CPU raises unaligned exception (if only it would raise the segfault as higher priority, before the unaligned!)
4. unaligned exception handler attempts to simulate the unaligned access with multiple partial read/write ops
5. CPU raises MMU exception on the read/write by the unaligned handler
6. kernel segfault handler looks up faulting address, it is in the unaligned exception handler, which has no fixup.
7. no fixup -> failure

So, I suppose the question is - where in the sequence is the true failure?

Clearly LTP thinks it's ok to pass unaligned pointers to the kernel, suggesting (1) is fine - thus my question about alignment rules in the ABI.

Do we need fixups on the unaligned handler itself? This will be ugly ugly ugly.

Or, some way of tracing the segfault back through the unaligned exception and to the root cause (the get/put-user), and call that fixup as required?

Quoted 9 lines · John Williams

Quoted 7 lines · Arnd Bergmann

The solution then is to handle fixups from the unaligned exception handler if you come from the kernel. That should fix the three text cases.

I don't fully understand your exception handling there, but I think you also need to add code checking for __range_ok() to your unaligned handler, to prevent malicious user space code from accessing the kernel through unaligned pointers.

Quoted 2 lines

Just to try to clarify - are there any alignment rules in the ABI on user-space pointers (which end up going to get/put_user)?

The kernel normally expects aligned input from user space, but I guess it can't hurt to handle it anyway. arch/mips/kernel/alignment.c seems to handle that case. Maybe Ralf can give some more insight.

Quoted 13 lines

It seems the failure path is like this:

1. userspace passes unaligned pointer
2. get_user attempts to access
3. CPU raises unaligned exception (if only it would raise the segfault as higher priority, before the unaligned!)
4. unaligned exception handler attempts to simulate the unaligned access with multiple partial read/write ops
5. CPU raises MMU exception on the read/write by the unaligned handler
6. kernel segfault handler looks up faulting address, it is in the unaligned exception handler, which has no fixup.
7. no fixup -> failure

Right.

Quoted 1 line

So, I suppose the question is - where in the sequence is the true failure?

I think in step 4. AFIACT, the kernel must do a number of checks on accesses to random pointers.

Quoted 2 lines

Clearly LTP thinks it's ok to pass unaligned pointers to the kernel, suggesting (1) is fine - thus my question about alignment rules in the ABI.

No, LTP thinks it should get a -EFAULT error code for that access. It does specify whether it expects this because of an unaligned address or because of an invalid page.

Quoted 3 lines

Do we need fixups on the unaligned handler itself? This will be ugly ugly ugly.

That's what ARM does. You don't have to do it from assembly though, implementing it in C is probably easier.

Quoted 3 lines

Or, some way of tracing the segfault back through the unaligned exception and to the root cause (the get/put-user), and call that fixup as required?

Yes, I guess that would have to look roughly like this:

int emulate_insn(struct pt_regs *regs, unsigned long addr, unsigned long len)

{
	/* use inline assembly with fixups here, return -EFAULT on bad addr */		
}

void alignment_exception(struct pt_regs *regs, unsigned long addr, unsigned long len)

{
	const struct exception_table_entry *fixup;
	int err;

	if (user_mode(regs)) {
		if (!access_ok(addr, len))
			goto segv;
		if (emulate_insn(regs) == -EFAULT))
			goto segv;
	} else {
		if (!access_ok(addr, len))
			goto fixup;
		if (emulate_insn(regs, addr, len) == -EFAULT))
			goto fixup;
	return;

fixup:

	fixup = search_exception_tables(regs->ip);
	if (!fixup)
		goto segv;

	regs->ip = fixup->fixup;
	return;

segv:

	force_sig(SIGSEGV, current));
}

Quoted 10 lines · John Williams

Quoted 7 lines · Arnd Bergmann

The solution then is to handle fixups from the unaligned exception handler if you come from the kernel. That should fix the three text cases.

I don't fully understand your exception handling there, but I think you also need to add code checking for __range_ok() to your unaligned handler, to prevent malicious user space code from accessing the kernel through unaligned pointers.

Quoted 2 lines

Just to try to clarify - are there any alignment rules in the ABI on user-space pointers (which end up going to get/put_user)?

The kernel normally expects aligned input from user space, but I guess it can't hurt to handle it anyway. arch/mips/kernel/alignment.c seems to handle that case. Maybe Ralf can give some more insight.

Quoted 12 lines

It seems the failure path is like this:

1. userspace passes unaligned pointer
2. get_user attempts to access
3. CPU raises unaligned exception (if only it would raise the segfault as higher priority, before the unaligned!)
4. unaligned exception handler attempts to simulate the unaligned access with multiple partial read/write ops
5. CPU raises MMU exception on the read/write by the unaligned handler
6. kernel segfault handler looks up faulting address, it is in the unaligned exception handler, which has no fixup.
7. no fixup -> failure

Right.

Quoted 1 line

So, I suppose the question is - where in the sequence is the true failure?

I think in step 4. AFIACT, the kernel must do a number of checks on accesses to random pointers.

Quoted 2 lines

Clearly LTP thinks it's ok to pass unaligned pointers to the kernel, suggesting (1) is fine - thus my question about alignment rules in the ABI.

No, LTP thinks it should get a -EFAULT error code for that access. It does specify whether it expects this because of an unaligned address or because of an invalid page.

Quoted 2 lines

Do we need fixups on the unaligned handler itself? This will be ugly ugly ugly.

That's what ARM does. You don't have to do it from assembly though, implementing it in C is probably easier.

Quoted 2 lines

I think in step 4. AFIACT, the kernel must do a number of checks on accesses to random pointers.

Yes; but the checks that unaligned.c does on MIPS are no different from any other get_user(), that is it ensures that the entire 8/16/32/64-bit access is in userspace. That's done using access_ok().

We've moved on. Handling miss-alignment is no longer a very relavent part of the syscall interface. So I think for the definition of the Linux ABI this should just be left as implementation defined behaviour but whatever an architecture does, it should be done consistently for all system interfaces.