linux-kernel - Re: [PATCH V2 0/7] usb-misc: sisusbvga: cleanup and bug fix

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]

Date:	Fri, 15 Jan 2016 09:51:49 -0800
From:	Joe Perches <joe@...ches.com>
To:	Peter Senna Tschudin <peter.senna@...labora.com>,
	thomas@...ischhofer.net, gregkh@...uxfoundation.org,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	sergei.shtylyov@...entembedded.com
Cc:	Masahiro Yamada <yamada.m@...panasonic.com>,
	Jason Cooper <jason@...edaemon.net>
Subject: Re: [PATCH V2 0/7] usb-misc: sisusbvga: cleanup and bug fix

(cc'ing Masahiro Yamada and Jason Cooper for objdiff)

On Fri, 2016-01-15 at 18:41 +0100, Peter Senna Tschudin wrote:
> The file drivers/usb/misc/sisusbvga/sisusb.c had many (192) coding style issues
> reported by checkpatch. This file also had a problematic error path in the
> probe function that could result in dereferencing a null pointer.
> 
> This patch series fix coding style issues and a problematic error path which
> could result in a null pointer dereference.
> 
> Patch 1 and 2 change whitespace only, patch 3 to 6 fix various coding style
> issues, and patch 7 fix a null pointer dereference bug.
> 
> Joe Perches suggested me to include objtdiff output for patches that are not
> supposed to make semantic changes, but it is not working well for me with gcc
> (GCC) 5.3.1 20151207 (Red Hat 5.3.1-2). Even compiling the same source code
> produces different output from objdump. The objdump command I'm using is from
> ./scripts/objdiff. See an example:

It seems gcc 5.3 isn't producing consistent output
for whitespace changes.

As far as I know, gcc has never made guarantees about
object output for the same input content.

> # A patch that should not make any semantic change
> $ cat /tmp/patch
>  diff --git a/drivers/usb/misc/sisusbvga/sisusb.c b/drivers/usb/misc/sisusbvga/sisusb.c
>  index 8efbaba..a48b086d 100644
>  --- a/drivers/usb/misc/sisusbvga/sisusb.c
>  +++ b/drivers/usb/misc/sisusbvga/sisusb.c
>  @@ -1353,7 +1353,7 @@ sisusb_testreadwrite(struct sisusb_usb_data *sisusb)
>       static char srcbuffer[] = { 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77 };
>       char destbuffer[10];
>       size_t dummy;
>  -    int i,j;
>  +    int i, j;
>   
>       sisusb_copy_memory(sisusb, srcbuffer, sisusb->vrambase, 7, &dummy);
> 
> # Lets compile and collect a dump based on linux-next/master
> $ git checkout linux-next/master
> $ md5sum drivers/usb/misc/sisusbvga/sisusb.c
> d6ffbd44f3f1cf81fd55ce84441ab889  drivers/usb/misc/sisusbvga/sisusb.c
> 
> $ make -j4 drivers/usb/misc/sisusbvga/sisusb.o
> $ objdump -D drivers/usb/misc/sisusbvga/sisusb.o| \
>   sed "s/^[[:space:]]\+[0-9a-f]\+//" > /tmp/base_dump
> 
> # Now let's apply the patch and collect other dump
> $ git apply /tmp/patch
> $ md5sum drivers/usb/misc/sisusbvga/sisusb.c
> 0b7d579c8ae2159f677c6a5c6efc4956  drivers/usb/misc/sisusbvga/sisusb.c
> 
> $ make -j4 drivers/usb/misc/sisusbvga/sisusb.o
> $ objdump -D drivers/usb/misc/sisusbvga/sisusb.o| \
>   sed "s/^[[:space:]]\+[0-9a-f]\+//" > /tmp/dump
> 
> # I was expecting the diff to be empty
> $ diff /tmp/base_dump /tmp/dump
>  9135,9136c9135
>  < :	8e 4d 31             	mov    0x31(%rbp),%cs
>  < :	46 00 00             	rex.RX add %r8b,(%rax)
>  ---
>  > :	25 c4 31 46 00       	and    $0x4631c4,%eax
>  9139c9138
>  < :	4b 00 00             	rex.WXB add %al,(%r8)
>  ---
>  > :	00 4b 00             	add    %cl,0x0(%rbx)
> 
> # But here is the interesting part. Even compiling the exact same source code
> # produces different results
> $ git checkout -- .
> $ md5sum drivers/usb/misc/sisusbvga/sisusb.c
> d6ffbd44f3f1cf81fd55ce84441ab889  drivers/usb/misc/sisusbvga/sisusb.c
> 
> $ make -j4 drivers/usb/misc/sisusbvga/sisusb.o
> $ objdump -D drivers/usb/misc/sisusbvga/sisusb.o| \
>   sed "s/^[[:space:]]\+[0-9a-f]\+//" > /tmp/base_dump_again
> 
> $ diff /tmp/base_dump /tmp/base_dump_again 
>  9135,9136c9135,9136
>  < :	8e 4d 31             	mov    0x31(%rbp),%cs
>  < :	46 00 00             	rex.RX add %r8b,(%rax)
>  ---
>  > :	de 10                	ficom  (%rax)
>  > :	33 46 00             	xor    0x0(%rsi),%eax
>  9139c9139
>  < :	4b 00 00             	rex.WXB add %al,(%r8)
>  ---
>  > :	00 4b 00             	add    %cl,0x0(%rbx)
> 
> Peter Senna Tschudin (7):
>   usb-misc: sisusbvga: Fix coding style: horizontal whitespace changes
>   usb-misc: sisusbvga: Fix coding style: vertical whitespace changes
>   usb-misc: sisusbvga: Fix coding style: braces, parenthesis, comment
>   usb-misc: sisusbvga: Fix coding style: remove assignment from if tests
>   usb-misc: sisusbvga: Remove null test before calls to kfree()
>   usb-misc: sisusbvga: Remove memory allocation logs
>   usb-misc: sisusbvga: fix error path
> 
>  drivers/usb/misc/sisusbvga/sisusb.c | 1543 +++++++++++++++++------------------
>  1 file changed, 752 insertions(+), 791 deletions(-)
>