Gaah. Re-sending this for the kernel mailing list just for posterity.
I keep replying to emails that had the mailing list address wrong, and
then my reply will have it wrong too.
I will learn to fix up the address just in time for this thread to die
out, I suspect.
Linus
On Mon, Jan 7, 2019 at 10:02 AM Linus Torvalds
<[email protected]> wrote:
>
> On Sun, Jan 6, 2019 at 8:05 PM Guenter Roeck <[email protected]> wrote:
> >
> > Of the above, my test system boots images for the following architectures
> > in qemu.
> >
> > - mips (32/64 bit, big/little endian)
> > - nios2
> > - openrisc
> > - xtensa (mmu and nommu)
>
> So most of those are "only" the "macro arguments used twice" problem
> (although openrisc also does the "arguments not quoted right"). That
> doesn't cause problems with the new commit, it's an independent issue
> that could cause random problems elsewhere
>
> The nios2 access_ok() case is the same bug as alpha has, but it turns
> out to be hidden by the fact that the user/kernel limit is at
> 0x80000000, but nios2 does:
>
> # define TASK_SIZE 0x7FFF0000UL
>
> so it doesn't actually allow anything close to the top of the user
> address space anyway. So the access_ok() check uses a different limit
> than the TASK_SIZE, which is odd, but does hide the "last byte of the
> user address space" bug.
>
> That may be intentional, and regardless, it's generally a good idea to
> not allow mapping of the last page in the address space, exactly to
> avoid the border conditions.
>
> MIPS has some of the same "saved by mistake" behavior, but in that
> case it really looks to be pure luck, not design. In particular,
> MIPS32 has
>
> #ifdef CONFIG_32BIT
> #ifdef CONFIG_KVM_GUEST
> /* User space process size is limited to 1GB in KVM Guest Mode */
> #define TASK_SIZE 0x3fff8000UL
> #else
> /*
> * User space process size: 2GB. This is hardcoded into a few places,
> * so don't change it unless you know what you are doing.
> */
> #define TASK_SIZE 0x80000000UL
> #endif
>
> and I suspect you tested MIPS32 with that KVM_GUEST config option.
>
> Because MIPS32 with TASK_SIZE 0x80000000UL really looks like it has
> the off-by-one error that I think makes access_ok() fail for the "last
> byte of the user address space" case.
>
> HOWEVER. MIPS32 is actually going to boot for that case with the
> recent patches for a simple other accidental reason: despite the
> access_ok() bug, it will never trigger it in strncpy_from_user(). Why?
> Because MIPS doesn't use the generic version, but its own hardcoded
> assembler one.
>
> I suspect the MIPS assembler version is actually *worse* than the
> generic one (it looks like it does things one byte at a time), but it
> hides the bug in access_ok()...
>
> The other architctures you tested only have the "technically wrong,
> but works" bugs that don't matter for the new stricter access_ok()
> tests.
>
> Linus
On Mon, Jan 07, 2019 at 10:05:12AM -0800, Linus Torvalds wrote:
> Gaah. Re-sending this for the kernel mailing list just for posterity.
> I keep replying to emails that had the mailing list address wrong, and
> then my reply will have it wrong too.
>
> I will learn to fix up the address just in time for this thread to die
> out, I suspect.
>
> Linus
>
> On Mon, Jan 7, 2019 at 10:02 AM Linus Torvalds
> <[email protected]> wrote:
> >
> > On Sun, Jan 6, 2019 at 8:05 PM Guenter Roeck <[email protected]> wrote:
> > >
> > > Of the above, my test system boots images for the following architectures
> > > in qemu.
> > >
> > > - mips (32/64 bit, big/little endian)
> > > - nios2
> > > - openrisc
> > > - xtensa (mmu and nommu)
> >
> > So most of those are "only" the "macro arguments used twice" problem
> > (although openrisc also does the "arguments not quoted right"). That
> > doesn't cause problems with the new commit, it's an independent issue
> > that could cause random problems elsewhere
Linus, Thanks for pointing this out and Guenter thanks for testing.
I ack the issue on OpenRISC and will send a patch for review in a day or so.
I'll also do an audit of openrisc to see if we have similar issues.
Next to access_ok() is __addr_ok() which seems to have the similar quoting
issue, but I don't see it being used anywhere, it's also defined and not
used on: arm, x86 and csky. It is used on SH.
OK to remove __addr_ok()?
-Stafford