linux-kernel - Re: arm64: fp-stress: BUG: KFENCE: memory corruption in fpsimd_release

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]

Message-ID: <2d9a04d8-c09e-49aa-95eb-32b4679f7eba@kili.mountain>
Date:   Wed, 17 May 2023 22:30:29 +0300
From:   Dan Carpenter <dan.carpenter@...aro.org>
To:     Naresh Kamboju <naresh.kamboju@...aro.org>
Cc:     "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        linux-stable <stable@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        lkft-triage@...ts.linaro.org, Mark Brown <broonie@...nel.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, Arnd Bergmann <arnd@...db.de>,
        Shuah Khan <shuah@...nel.org>,
        Anders Roxell <anders.roxell@...aro.org>
Subject: Re: arm64: fp-stress: BUG: KFENCE: memory corruption in
 fpsimd_release_task

I don't know this code at all so probably this is dumb...  I don't
undestand how vec_set_vector_length() ensures that sme_state_size()
stays in sync with the actual size allocated in sme_alloc()

arch/arm64/kernel/fpsimd.c
   847  int vec_set_vector_length(struct task_struct *task, enum vec_type type,
   848                            unsigned long vl, unsigned long flags)
                                               ^^^
"vl" comes from the user and is 0-u16max.

   849  {
   850          if (flags & ~(unsigned long)(PR_SVE_VL_INHERIT |
   851                                       PR_SVE_SET_VL_ONEXEC))
   852                  return -EINVAL;
   853  
   854          if (!sve_vl_valid(vl))

valid values are '16-8192'

   855                  return -EINVAL;
   856  
   857          /*
   858           * Clamp to the maximum vector length that VL-agnostic code
   859           * can work with.  A flag may be assigned in the future to
   860           * allow setting of larger vector lengths without confusing
   861           * older software.
   862           */
   863          if (vl > VL_ARCH_MAX)
   864                  vl = VL_ARCH_MAX;

Now 16-256'

   865  
   866          vl = find_supported_vector_length(type, vl);

type is ARM64_VEC_SVE.  I've looked at this function for a while and
I don't see anything which ensures that "vl" is less than the current
value.

   867  
   868          if (flags & (PR_SVE_VL_INHERIT |
   869                       PR_SVE_SET_VL_ONEXEC))
   870                  task_set_vl_onexec(task, type, vl);
   871          else
   872                  /* Reset VL to system default on next exec: */
   873                  task_set_vl_onexec(task, type, 0);
   874  
   875          /* Only actually set the VL if not deferred: */
   876          if (flags & PR_SVE_SET_VL_ONEXEC)

Assume the PR_SVE_SET_VL_ONEXEC flag is not set.

   877                  goto out;
   878  
   879          if (vl == task_get_vl(task, type))

This checks if the flag is == and if so we are done.

   880                  goto out;
   881  
   882          /*
   883           * To ensure the FPSIMD bits of the SVE vector registers are preserved,
   884           * write any live register state back to task_struct, and convert to a
   885           * regular FPSIMD thread.
   886           */
   887          if (task == current) {
   888                  get_cpu_fpsimd_context();
   889  
   890                  fpsimd_save();
   891          }
   892  
   893          fpsimd_flush_task_state(task);
   894          if (test_and_clear_tsk_thread_flag(task, TIF_SVE) ||
   895              thread_sm_enabled(&task->thread)) {
   896                  sve_to_fpsimd(task);
   897                  task->thread.fp_type = FP_STATE_FPSIMD;
   898          }
   899  
   900          if (system_supports_sme() && type == ARM64_VEC_SME) {
   901                  task->thread.svcr &= ~(SVCR_SM_MASK |
   902                                         SVCR_ZA_MASK);
   903                  clear_thread_flag(TIF_SME);
   904          }
   905  
   906          if (task == current)
   907                  put_cpu_fpsimd_context();
   908  
   909          /*
   910           * Force reallocation of task SVE and SME state to the correct
   911           * size on next use:
   912           */
   913          sve_free(task);
   914          if (system_supports_sme() && type == ARM64_VEC_SME)
   915                  sme_free(task);
   916  
   917          task_set_vl(task, type, vl);

"vl" is set here.  This is fine if we are setting it to a smaller value,
but if we are setting it to a larger value then I think we need to
realloc the ->sme_state buffer.

When we call sme_alloc() it will say the buffer is already allocated
and just zero out what we need for "vl", but the existing buffer is too
small.

   918  
   919  out:
   920          update_tsk_thread_flag(task, vec_vl_inherit_flag(type),
   922  
   923          return 0;
   924  }

regards,
dan carpenter