Re: arch/arm64/kernel/compat_alignment.c:333 do_compat_alignment_fixup() error: uninitialized symbol 'tinst2'.
From: Ard Biesheuvel
Date: Mon Apr 03 2023 - 05:11:03 EST
On Mon, 3 Apr 2023 at 10:34, Dan Carpenter <error27@xxxxxxxxx> wrote:
>
> 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:
>
My bad (and this illustrates my point about not deviating from the
original when there is a need for two copies of the code to exist in
the same tree, only not in the way I thought)
So the ARM code is correct, and the arm64 version deviates from it,
and introduces the issue you are reporting.
I agree that initializing tinst2 to 0 is the appropriate course of action here.
Thanks,
Ard.
> 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
>