linux-kernel - Re: [PATCH v2] powerpc/uaccess: simplify the get_fs() set

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] [day] [month] [year] [list]

Date:   Thu, 06 Aug 2020 22:33:01 +1000
From:   Michael Ellerman <mpe@...erman.id.au>
To:     Christophe Leroy <christophe.leroy@...roup.eu>,
        Christophe Leroy <christophe.leroy@....fr>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>
Cc:     linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v2] powerpc/uaccess: simplify the get_fs() set_fs() logic

Christophe Leroy <christophe.leroy@...roup.eu> writes:
> Le 25/07/2020 à 13:22, Michael Ellerman a écrit :
>> Hi Christophe,
>> 
>> Unfortunately this would collide messily with "uaccess: remove
>> segment_eq" in linux-next, so I'll ask you to do a respin based on that,
>> some comments below.
>
> Done, sent as v3, together with the 2 patchs from Linux next to get it 
> build and boot.

Thanks.

>> Christophe Leroy <christophe.leroy@....fr> writes:
>>> On powerpc, we only have USER_DS and KERNEL_DS
>>>
>>> Today, this is managed as an 'unsigned long' data space limit
>>> which is used to compare the passed address with, plus a bit
>>> in the thread_info flags that is set whenever modifying the limit
>>> to enable the verification in addr_limit_user_check()
>>>
>>> The limit is either the last address of user space when USER_DS is
>>> set, and the last address of address space when KERNEL_DS is set.
>>> In both cases, the limit is a compiletime constant.
>>>
>>> get_fs() returns the limit, which is part of thread_info struct
>>> set_fs() updates the limit then set the TI_FSCHECK flag.
>>> addr_limit_user_check() check the flag, and if it is set it checks
>>> the limit is the user limit, then unsets the TI_FSCHECK flag.
>>>
>>> In addition, when the flag is set the syscall exit work is involved.
>>> This exit work is heavy compared to normal syscall exit as it goes
>>> through normal exception exit instead of the fast syscall exit.
>>>
>>> Rename this TI_FSCHECK flag to TIF_KERNEL_DS flag which tells whether
>>> KERNEL_DS or USER_DS is set. Get mm_segment_t be redifined as a bool
>>> struct that is either false (for USER_DS) or true (for KERNEL_DS).
>>> When TIF_KERNEL_DS is set, the limit is ~0UL. Otherwise it is
>>> TASK_SIZE_USER (resp TASK_SIZE_USER64 on PPC64). When KERNEL_DS is
>>> set, there is no range to check. Define TI_FSCHECK as an alias to
>>> TIF_KERNEL_DS.
>> 
>> I'd rather avoid the "DS" name any more than we have to. Maybe it means
>> "data space" but that's not a very common term.
>
> I thought it was a reference to the ds/fs/gs ... segment registers in 
> the 8086 ?

Yes.

In your changelog you used "data space limit", so I thought you were
trying to retrospectively redefine the "DS" acronym to mean that.

cheers