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/[email protected]/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 <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Link: https://lore.kernel.org/r/[email protected]/
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.
Most distro kernels are going to automatically zero out stack variables
like tinst2 to prevent undefined behavior.
Presumably this is a fast path. So setting "u16 tinst2 = 0;" does not
affect runtime speed for distro kernels and it might be the best
solution.
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On Mon, 3 Apr 2023 at 08:29, Dan Carpenter <[email protected]> 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/[email protected]/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 <[email protected]>
> | Reported-by: Dan Carpenter <[email protected]>
> | Link: https://lore.kernel.org/r/[email protected]/
>
> 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.
> Most distro kernels are going to automatically zero out stack variables
> like tinst2 to prevent undefined behavior.
>
instr is already zeroed out.
> Presumably this is a fast path. So setting "u16 tinst2 = 0;" does not
> affect runtime speed for distro kernels and it might be the best
> solution.
>
Performance is not an issue here - this is a misalignment fixup
handler, which we copied from the 32-bit ARM tree only for compat
reasons, but anyone who cares about performance would not use
misaligned accesses or even run 32-bit code on a 64-bit system.
However, given that this code originates in the arch/arm tree, I am
reluctant make such 'correctness' tweaks while the logic is sound and
unambiguous.
On Mon, Apr 03, 2023 at 10:03:15AM +0200, Ard Biesheuvel wrote:
> On Mon, 3 Apr 2023 at 08:29, Dan Carpenter <[email protected]> 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/[email protected]/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 <[email protected]>
> > | Reported-by: Dan Carpenter <[email protected]>
> > | Link: https://lore.kernel.org/r/[email protected]/
> >
> > 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
On Mon, 3 Apr 2023 at 10:34, Dan Carpenter <[email protected]> wrote:
>
> On Mon, Apr 03, 2023 at 10:03:15AM +0200, Ard Biesheuvel wrote:
> > On Mon, 3 Apr 2023 at 08:29, Dan Carpenter <[email protected]> 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/[email protected]/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 <[email protected]>
> > > | Reported-by: Dan Carpenter <[email protected]>
> > > | Link: https://lore.kernel.org/r/[email protected]/
> > >
> > > 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
>