linux-kernel - Re: Address fixups in arch/x86/entry/vdso/vma.c

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]

Message-ID: <05B2C27D-9C2F-483C-88D5-B24ADFECAA16@zytor.com>
Date: Mon, 21 Jul 2025 15:36:12 -0700
From: "H. Peter Anvin" <hpa@...or.com>
To: Thomas Gleixner <tglx@...utronix.de>, Andy Lutomirski <luto@...nel.org>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>
CC: x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: Address fixups in arch/x86/entry/vdso/vma.c

On July 21, 2025 3:02:25 PM PDT, Thomas Gleixner <tglx@...utronix.de> wrote:
>On Thu, Jul 17 2025 at 10:34, H. Peter Anvin wrote:
>> One of the thing that my mind flagged was this:
>>
>> static void vdso_fix_landing(const struct vdso_image *image,
>>                 struct vm_area_struct *new_vma)
>> {
>>         if (in_ia32_syscall() && image == &vdso_image_32) {
>>                 struct pt_regs *regs = current_pt_regs();
>>                 unsigned long vdso_land = image->sym_int80_landing_pad;
>>                 unsigned long old_land_addr = vdso_land +
>>                         (unsigned long)current->mm->context.vdso;
>>
>>                 /* Fixing userspace landing - look at do_fast_syscall_32 */
>>                 if (regs->ip == old_land_addr)
>>                         regs->ip = new_vma->vm_start + vdso_land;
>>         }
>> }
>>
>> static int vdso_mremap(const struct vm_special_mapping *sm,
>>                 struct vm_area_struct *new_vma)
>> {
>>         const struct vdso_image *image =
>> 		current->mm->context.vdso_image;
>>
>>         vdso_fix_landing(image, new_vma);
>>         current->mm->context.vdso = (void __user *)new_vma->vm_start;
>>
>>         return 0;
>> }
>>
>>
>> --- ---
>>
>> This feels *way* more complicated than it should need to be.  It seems
>> to me that if the ip is inside the vdso at all, it would need to be
>> adjusted, regardless of if it in an ia32 system call or not, and if it
>> is at the specific landing spot or not.
>
>In practice the only situation where ret->ip can be inside the VDSO is
>when the remap syscall was invoked as IA32 syscall and the VDSO image is
>a 32-bit image. So this check is pretty much paranoia.
>
>> It is possible that it doesn't *matter*, but that's not really a good
>> reason to make the code more complex.
>>
>> I came up with the following version as an alternative; I would be
>> interesting to hear what you think.
>>
>> (Also, (unsigned long)current->mm->context.vdso occurs *all over the
>> place*, but there is also a macro defined for it (VDSO_CURRENT_BASE, in
>> <asm/elf.h>. My personal preference would be to replace both with an
>> inline function.)
>
>No objections, but in a seperate patch.
>
>> If you don't think I'm missing something, I would like to do something
>> like this:
>>
>>
>> static inline void
>> vdso_fix_address(unsigned long *ptr, const struct vdso_image *image,
>>                  unsigned long from, unsigned long to)
>> {
>>         if (!image)		/* For potential uses elsewhere */
>
>Aside of tail comments being horrible, this comment is just useless
>gunk.
>
>>                 return;
>>
>>         unsigned long offset = *ptr - from;
>
>Why on earth do you need to hand in the pt_regs->ip pointer instead of
>using pt_regs->ip here at the usage side? Just to make the code even
>less understandable than the original one?
>
>>         if (offset < image->size)
>>                 *ptr = offset + to;
>> }
>>
>> static int vdso_mremap(const struct vm_special_mapping *sm,
>>                 struct vm_area_struct *new_vma)
>> {
>>         vdso_fix_address(&current_pt_regs()->ip,
>>                          current->mm->context.vdso_image,
>>                          vdso_current_base(),
>>                          new_vma->vm_start);
>
>TBH, this is incomprehensible garbage. If you want to simplify the whole
>thing, then why not doing the obvious:
>
>static int vdso_mremap(const struct vm_special_mapping *sm, struct vm_area_struct *new_vma)
>{
>	const struct vdso_image *image = current->mm->context.vdso_image;
>
>        if (image) {
>		struct pt_regs *regs = current_pt_regs();
>        	unsigned long offset = regs->ip - vdso_current_base();
>
>                /* Add a useful comment */
>                if (offset < image->size)
>                	regs->ip = new_vma->vm_start + offset;
>        }
>
>	current->mm->context.vdso = (void __user *)new_vma->vm_start;
>}
>
>Thanks,
>
>        tglx

Yes, it was more complicated than necessary, because at first I was looking at reusing the routine in some other places I was working on, but it doesn't seem to be necessary, so no reason to complicate things. 

Thanks for confirming what I suspected.