Re: arch/arm64/kernel/compat_alignment.c:333 do_compat_alignment_fixup() error: uninitialized symbol 'tinst2'.

[Dan Carpenter]

On Mon, Apr 03, 2023 at 10:03:15AM +0200, Ard Biesheuvel wrote:
> On Mon, 3 Apr 2023 at 08:29, Dan Carpenter <error27@xxxxxxxxx> wrote:
> >
> > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > head:   00c7b5f4ddc5b346df62b757ec73f9357bb452af
> > commit: 3fc24ef32d3b9368f4c103dcd21d6a3f959b4870 arm64: compat: Implement misalignment fixups for multiword loads
> > config: arm64-randconfig-m041-20230329 (https://download.01.org/0day-ci/archive/20230402/202304021214.gekJ8yRc-lkp@xxxxxxxxx/config)
> > compiler: aarch64-linux-gcc (GCC) 12.1.0
> >
> > If you fix the issue, kindly add following tag where applicable
> > | Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > | Reported-by: Dan Carpenter <error27@xxxxxxxxx>
> > | Link: https://lore.kernel.org/r/202304021214.gekJ8yRc-lkp@xxxxxxxxx/
> >
> > smatch warnings:
> > arch/arm64/kernel/compat_alignment.c:333 do_compat_alignment_fixup() error: uninitialized symbol 'tinst2'.
> >
> > vim +/tinst2 +333 arch/arm64/kernel/compat_alignment.c
> >
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  310  int do_compat_alignment_fixup(unsigned long addr, struct pt_regs *regs)
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  311  {
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  312   union offset_union offset;
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  313   unsigned long instrptr;
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  314   int (*handler)(unsigned long addr, u32 instr, struct pt_regs *regs);
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  315   unsigned int type;
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  316   u32 instr = 0;
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  317   u16 tinstr = 0;
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  318   int isize = 4;
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  319   int thumb2_32b = 0;
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  320   int fault;
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  321
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  322   instrptr = instruction_pointer(regs);
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  323
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  324   if (compat_thumb_mode(regs)) {
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  325           __le16 __user *ptr = (__le16 __user *)(instrptr & ~1);
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  326
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  327           fault = alignment_get_thumb(regs, ptr, &tinstr);
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  328           if (!fault) {
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  329                   if (IS_T32(tinstr)) {
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  330                           /* Thumb-2 32-bit */
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  331                           u16 tinst2;
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01  332                           fault = alignment_get_thumb(regs, ptr + 1, &tinst2);
> > 3fc24ef32d3b93 Ard Biesheuvel 2022-07-01 @333                           instr = ((u32)tinstr << 16) | tinst2;
> >
> > Smatch is complaining that there is no error checking to see if the
> > copy_from_user() fails in alignment_get_thumb.  Eventually the syzbot
> > will learn to detect this as well.
> >
> 
> That shouldn't matter.
> 
>         u16 instr = 0;
>         int fault;
> 
>         if (user_mode(regs))
>                 fault = get_user(instr, ip);
>         else
>                 fault = get_kernel_nofault(instr, ip);
> 
>         *inst = __mem_to_opcode_thumb16(instr);
> 
> So the *inst assignment is never ambiguous here, regardless of whether
> get_user() fails. The value could be wrong (i.e., get_user may have
> failed after reading only one byte) but the value is never
> uninitialized. Then, the fault value is always propagated so the
> calling function will not succeed spuriously when this happens.
> 

I don't know what code that is...  We are looking at different code.
For me on linux-next it looks like this:

arch/arm64/kernel/compat_alignment.c
   297  static int alignment_get_thumb(struct pt_regs *regs, __le16 __user *ip, u16 *inst)
   298  {
   299          __le16 instr = 0;
   300          int fault;
   301  
   302          fault = get_user(instr, ip);
   303          if (fault)
   304                  return fault;

*inst not initialized.

   305  
   306          *inst = __le16_to_cpu(instr);
   307          return 0;
   308  }
   309  
   310  int do_compat_alignment_fixup(unsigned long addr, struct pt_regs *regs)
   311  {
   312          union offset_union offset;
   313          unsigned long instrptr;
   314          int (*handler)(unsigned long addr, u32 instr, struct pt_regs *regs);
   315          unsigned int type;
   316          u32 instr = 0;
   317          u16 tinstr = 0;
   318          int isize = 4;
   319          int thumb2_32b = 0;
   320          int fault;
   321  
   322          instrptr = instruction_pointer(regs);
   323  
   324          if (compat_thumb_mode(regs)) {
   325                  __le16 __user *ptr = (__le16 __user *)(instrptr & ~1);
   326  
   327                  fault = alignment_get_thumb(regs, ptr, &tinstr);
   328                  if (!fault) {
   329                          if (IS_T32(tinstr)) {
   330                                  /* Thumb-2 32-bit */
   331                                  u16 tinst2;
   332                                  fault = alignment_get_thumb(regs, ptr + 1, &tinst2);
   333                                  instr = ((u32)tinstr << 16) | tinst2;
                                                                      ^^^^^^
Uninitialized variable here.

   334                                  thumb2_32b = 1;
   335                          } else {
   336                                  isize = 2;
   337                                  instr = thumb2arm(tinstr);
   338                          }
   339                  }
   340          } else {

regards,
dan carpenter