| 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
| ||
|
Message-ID: <589c4ea1-95e0-4e3e-9567-a4278860d256@kili.mountain>
Date: Mon, 3 Apr 2023 11:34:27 +0300
From: Dan Carpenter <error27@...il.com>
To: Ard Biesheuvel <ardb@...nel.org>
Cc: oe-kbuild@...ts.linux.dev, lkp@...el.com,
oe-kbuild-all@...ts.linux.dev, linux-kernel@...r.kernel.org,
Catalin Marinas <catalin.marinas@....com>,
Arnd Bergmann <arnd@...db.de>
Subject: Re: arch/arm64/kernel/compat_alignment.c:333
do_compat_alignment_fixup() error: uninitialized symbol 'tinst2'.
On Mon, Apr 03, 2023 at 10:03:15AM +0200, Ard Biesheuvel wrote:
> On Mon, 3 Apr 2023 at 08:29, Dan Carpenter <error27@...il.com> 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@intel.com/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@...el.com>
> > | Reported-by: Dan Carpenter <error27@...il.com>
> > | Link: https://lore.kernel.org/r/202304021214.gekJ8yRc-lkp@intel.com/
> >
> > 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
Powered by blists - more mailing lists