[PATCH] USB: sisusbvga: Fix left shifting a possible negative value

[PATCH] USB: sisusbvga: Fix left shifting a possible negative value

[Changming Liu]

The char buffer buf, accepts user data which might be negative value and
the content is left shifted to form an unsigned integer.

Since left shifting a negative value is undefined behavior, thus change
the char to u8 to fix this

Signed-off-by: Changming Liu <liu.changm@northeastern.edu>
---
 drivers/usb/misc/sisusbvga/sisusb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/misc/sisusbvga/sisusb.c b/drivers/usb/misc/sisusbvga/sisusb.c
index fc8a5da4a07c..0734e6dd9386 100644
--- a/drivers/usb/misc/sisusbvga/sisusb.c
+++ b/drivers/usb/misc/sisusbvga/sisusb.c
@@ -761,7 +761,7 @@ static int sisusb_write_mem_bulk(struct sisusb_usb_data *sisusb, u32 addr,
        u8   swap8, fromkern = kernbuffer ? 1 : 0;
        u16  swap16;
        u32  swap32, flag = (length >> 28) & 1;
-       char buf[4];
+       u8 buf[4];

        /* if neither kernbuffer not userbuffer are given, assume
         * data in obuf
--
2.17.1

Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value

[Greg KH]

On Wed, May 20, 2020 at 06:06:50PM +0000, Changming Liu wrote:
> The char buffer buf, accepts user data which might be negative value and
> the content is left shifted to form an unsigned integer.
> 
> Since left shifting a negative value is undefined behavior, thus change
> the char to u8 to fix this
> 
> Signed-off-by: Changming Liu <liu.changm@northeastern.edu>
> ---
>  drivers/usb/misc/sisusbvga/sisusb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/misc/sisusbvga/sisusb.c b/drivers/usb/misc/sisusbvga/sisusb.c
> index fc8a5da4a07c..0734e6dd9386 100644
> --- a/drivers/usb/misc/sisusbvga/sisusb.c
> +++ b/drivers/usb/misc/sisusbvga/sisusb.c
> @@ -761,7 +761,7 @@ static int sisusb_write_mem_bulk(struct sisusb_usb_data *sisusb, u32 addr,
>         u8   swap8, fromkern = kernbuffer ? 1 : 0;
>         u16  swap16;
>         u32  swap32, flag = (length >> 28) & 1;
> -       char buf[4];
> +       u8 buf[4];

Do we also need to change the kernbuffer variable from char* to be u8*
as the same time to solve the same potential issue?

thanks,

greg k-h

RE: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value

[Changming Liu]

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Thursday, May 21, 2020 3:36 AM
> To: Changming Liu <liu.changm@northeastern.edu>
> Cc: thomas@winischhofer.net; linux-usb@vger.kernel.org
> Subject: Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value
> 
> On Wed, May 20, 2020 at 06:06:50PM +0000, Changming Liu wrote:
> > The char buffer buf, accepts user data which might be negative value and
> > the content is left shifted to form an unsigned integer.
> >
> > Since left shifting a negative value is undefined behavior, thus change
> > the char to u8 to fix this
> >
> > Signed-off-by: Changming Liu <liu.changm@northeastern.edu>
> > ---
> >  drivers/usb/misc/sisusbvga/sisusb.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/misc/sisusbvga/sisusb.c
> b/drivers/usb/misc/sisusbvga/sisusb.c
> > index fc8a5da4a07c..0734e6dd9386 100644
> > --- a/drivers/usb/misc/sisusbvga/sisusb.c
> > +++ b/drivers/usb/misc/sisusbvga/sisusb.c
> > @@ -761,7 +761,7 @@ static int sisusb_write_mem_bulk(struct
> sisusb_usb_data *sisusb, u32 addr,
> >         u8   swap8, fromkern = kernbuffer ? 1 : 0;
> >         u16  swap16;
> >         u32  swap32, flag = (length >> 28) & 1;
> > -       char buf[4];
> > +       u8 buf[4];
> 
> Do we also need to change the kernbuffer variable from char* to be u8*
> as the same time to solve the same potential issue?
> 

This is a very good point, sorry I didn't notice this.
Indeed, according to the caller of sisusb_copy_memory, the wrapper of current function
there is no guarantee that each char in kernbuffer is positive.

However, it seems if we change the function argument type directly from char* to u8*, 
Other parts that call this function e.g. in sisusb_copy_memory 
or uses this pointer e.g. line 770,line 883 must change accordingly.
Looks like many force casts which doesn't look too necessary.

I wonder how about just force casting the content of kernbuffer when it's read in line 823 to line 829
from char to u8? This seems explicitly fix this bug.

Best,
Changming

Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value

[Greg KH]

On Thu, May 21, 2020 at 05:56:44PM +0000, Changming Liu wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: Thursday, May 21, 2020 3:36 AM
> > To: Changming Liu <liu.changm@northeastern.edu>
> > Cc: thomas@winischhofer.net; linux-usb@vger.kernel.org
> > Subject: Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value
> > 
> > On Wed, May 20, 2020 at 06:06:50PM +0000, Changming Liu wrote:
> > > The char buffer buf, accepts user data which might be negative value and
> > > the content is left shifted to form an unsigned integer.
> > >
> > > Since left shifting a negative value is undefined behavior, thus change
> > > the char to u8 to fix this
> > >
> > > Signed-off-by: Changming Liu <liu.changm@northeastern.edu>
> > > ---
> > >  drivers/usb/misc/sisusbvga/sisusb.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/misc/sisusbvga/sisusb.c
> > b/drivers/usb/misc/sisusbvga/sisusb.c
> > > index fc8a5da4a07c..0734e6dd9386 100644
> > > --- a/drivers/usb/misc/sisusbvga/sisusb.c
> > > +++ b/drivers/usb/misc/sisusbvga/sisusb.c
> > > @@ -761,7 +761,7 @@ static int sisusb_write_mem_bulk(struct
> > sisusb_usb_data *sisusb, u32 addr,
> > >         u8   swap8, fromkern = kernbuffer ? 1 : 0;
> > >         u16  swap16;
> > >         u32  swap32, flag = (length >> 28) & 1;
> > > -       char buf[4];
> > > +       u8 buf[4];
> > 
> > Do we also need to change the kernbuffer variable from char* to be u8*
> > as the same time to solve the same potential issue?
> > 
> 
> This is a very good point, sorry I didn't notice this.
> Indeed, according to the caller of sisusb_copy_memory, the wrapper of current function
> there is no guarantee that each char in kernbuffer is positive.
> 
> However, it seems if we change the function argument type directly from char* to u8*, 
> Other parts that call this function e.g. in sisusb_copy_memory 
> or uses this pointer e.g. line 770,line 883 must change accordingly.
> Looks like many force casts which doesn't look too necessary.
> 
> I wonder how about just force casting the content of kernbuffer when it's read in line 823 to line 829
> from char to u8? This seems explicitly fix this bug.

That will work, but how about just changing all instances of char to u8
throughout this driver to make sure everything is working properly that
way.  char should not be used as a type when copying around "raw" data
like this from user-to-device for these reasons.

thanks,

greg k-h

RE: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value

[Changming Liu]

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Friday, May 22, 2020 3:24 AM
> To: Changming Liu <liu.changm@northeastern.edu>
> Cc: thomas@winischhofer.net; linux-usb@vger.kernel.org
> Subject: Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value
> 
> On Thu, May 21, 2020 at 05:56:44PM +0000, Changming Liu wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH <gregkh@linuxfoundation.org>
> > > Sent: Thursday, May 21, 2020 3:36 AM
> > > To: Changming Liu <liu.changm@northeastern.edu>
> > > Cc: thomas@winischhofer.net; linux-usb@vger.kernel.org
> > > Subject: Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative
> value
> > >
> > > On Wed, May 20, 2020 at 06:06:50PM +0000, Changming Liu wrote:
> > > > The char buffer buf, accepts user data which might be negative value and
> > > > the content is left shifted to form an unsigned integer.
> > > >
> > > > Since left shifting a negative value is undefined behavior, thus change
> > > > the char to u8 to fix this
> > > >
> > > > Signed-off-by: Changming Liu <liu.changm@northeastern.edu>
> > > > ---
> > > >  drivers/usb/misc/sisusbvga/sisusb.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/usb/misc/sisusbvga/sisusb.c
> > > b/drivers/usb/misc/sisusbvga/sisusb.c
> > > > index fc8a5da4a07c..0734e6dd9386 100644
> > > > --- a/drivers/usb/misc/sisusbvga/sisusb.c
> > > > +++ b/drivers/usb/misc/sisusbvga/sisusb.c
> > > > @@ -761,7 +761,7 @@ static int sisusb_write_mem_bulk(struct
> > > sisusb_usb_data *sisusb, u32 addr,
> > > >         u8   swap8, fromkern = kernbuffer ? 1 : 0;
> > > >         u16  swap16;
> > > >         u32  swap32, flag = (length >> 28) & 1;
> > > > -       char buf[4];
> > > > +       u8 buf[4];
> > >
> > > Do we also need to change the kernbuffer variable from char* to be u8*
> > > as the same time to solve the same potential issue?
> > >
> >
> > This is a very good point, sorry I didn't notice this.
> > Indeed, according to the caller of sisusb_copy_memory, the wrapper of
> current function
> > there is no guarantee that each char in kernbuffer is positive.
> >
> > However, it seems if we change the function argument type directly from
> char* to u8*,
> > Other parts that call this function e.g. in sisusb_copy_memory
> > or uses this pointer e.g. line 770,line 883 must change accordingly.
> > Looks like many force casts which doesn't look too necessary.
> >
> > I wonder how about just force casting the content of kernbuffer when it's
> read in line 823 to line 829
> > from char to u8? This seems explicitly fix this bug.
> 
> That will work, but how about just changing all instances of char to u8
> throughout this driver to make sure everything is working properly that
> way.  char should not be used as a type when copying around "raw" data
> like this from user-to-device for these reasons.
> 

This is a clean sweep, from the perspective of security I find no reason against it.
Indeed, u8 is strictly better than char when there is no need for any value to be negative.
I'd be very honored to see this through. 

I wonder, by this driver, you mean this sisusbvga module or something else?
Forgive me for my limited understanding of the module since I've only read the code related to this bug.
Please let me know on what files do you want to apply this change. 
Or if you feel like doing this yourself please go ahead, 
I'm still a bit daunted by the scale of changes that need to be made frankly :p

Best, 
Changming Liu

Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value

[Greg KH]

On Fri, May 22, 2020 at 07:14:32PM +0000, Changming Liu wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: Friday, May 22, 2020 3:24 AM
> > To: Changming Liu <liu.changm@northeastern.edu>
> > Cc: thomas@winischhofer.net; linux-usb@vger.kernel.org
> > Subject: Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value
> > 
> > On Thu, May 21, 2020 at 05:56:44PM +0000, Changming Liu wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Greg KH <gregkh@linuxfoundation.org>
> > > > Sent: Thursday, May 21, 2020 3:36 AM
> > > > To: Changming Liu <liu.changm@northeastern.edu>
> > > > Cc: thomas@winischhofer.net; linux-usb@vger.kernel.org
> > > > Subject: Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative
> > value
> > > >
> > > > On Wed, May 20, 2020 at 06:06:50PM +0000, Changming Liu wrote:
> > > > > The char buffer buf, accepts user data which might be negative value and
> > > > > the content is left shifted to form an unsigned integer.
> > > > >
> > > > > Since left shifting a negative value is undefined behavior, thus change
> > > > > the char to u8 to fix this
> > > > >
> > > > > Signed-off-by: Changming Liu <liu.changm@northeastern.edu>
> > > > > ---
> > > > >  drivers/usb/misc/sisusbvga/sisusb.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/usb/misc/sisusbvga/sisusb.c
> > > > b/drivers/usb/misc/sisusbvga/sisusb.c
> > > > > index fc8a5da4a07c..0734e6dd9386 100644
> > > > > --- a/drivers/usb/misc/sisusbvga/sisusb.c
> > > > > +++ b/drivers/usb/misc/sisusbvga/sisusb.c
> > > > > @@ -761,7 +761,7 @@ static int sisusb_write_mem_bulk(struct
> > > > sisusb_usb_data *sisusb, u32 addr,
> > > > >         u8   swap8, fromkern = kernbuffer ? 1 : 0;
> > > > >         u16  swap16;
> > > > >         u32  swap32, flag = (length >> 28) & 1;
> > > > > -       char buf[4];
> > > > > +       u8 buf[4];
> > > >
> > > > Do we also need to change the kernbuffer variable from char* to be u8*
> > > > as the same time to solve the same potential issue?
> > > >
> > >
> > > This is a very good point, sorry I didn't notice this.
> > > Indeed, according to the caller of sisusb_copy_memory, the wrapper of
> > current function
> > > there is no guarantee that each char in kernbuffer is positive.
> > >
> > > However, it seems if we change the function argument type directly from
> > char* to u8*,
> > > Other parts that call this function e.g. in sisusb_copy_memory
> > > or uses this pointer e.g. line 770,line 883 must change accordingly.
> > > Looks like many force casts which doesn't look too necessary.
> > >
> > > I wonder how about just force casting the content of kernbuffer when it's
> > read in line 823 to line 829
> > > from char to u8? This seems explicitly fix this bug.
> > 
> > That will work, but how about just changing all instances of char to u8
> > throughout this driver to make sure everything is working properly that
> > way.  char should not be used as a type when copying around "raw" data
> > like this from user-to-device for these reasons.
> > 
> 
> This is a clean sweep, from the perspective of security I find no reason against it.
> Indeed, u8 is strictly better than char when there is no need for any value to be negative.
> I'd be very honored to see this through. 
> 
> I wonder, by this driver, you mean this sisusbvga module or something else?
> Forgive me for my limited understanding of the module since I've only read the code related to this bug.
> Please let me know on what files do you want to apply this change. 

The sisusbvga module, all of the files that make it up, in
drivers/usb/misc/sisusbvga/ are what I am referring to here.

> Or if you feel like doing this yourself please go ahead, 
> I'm still a bit daunted by the scale of changes that need to be made frankly :p

Nope, you can do this, it shouldn't be that hard.  Might take a few
patches, do it as a patch series, doing one logical change per patch.

If you have specific questions, please let us know!

thanks,

greg k-h

RE: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value

[Changming Liu]

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Saturday, May 23, 2020 2:14 AM
> To: Changming Liu <liu.changm@northeastern.edu>
> Cc: thomas@winischhofer.net; linux-usb@vger.kernel.org
> Subject: Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value
> 
> On Fri, May 22, 2020 at 07:14:32PM +0000, Changming Liu wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH <gregkh@linuxfoundation.org>
> > > Sent: Friday, May 22, 2020 3:24 AM
> > > To: Changming Liu <liu.changm@northeastern.edu>
> > > Cc: thomas@winischhofer.net; linux-usb@vger.kernel.org
> > > Subject: Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative
> value
> > >
> > > On Thu, May 21, 2020 at 05:56:44PM +0000, Changming Liu wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Greg KH <gregkh@linuxfoundation.org>
> > > > > Sent: Thursday, May 21, 2020 3:36 AM
> > > > > To: Changming Liu <liu.changm@northeastern.edu>
> > > > > Cc: thomas@winischhofer.net; linux-usb@vger.kernel.org
> > > > > Subject: Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative
> > > value
> > > > >
> > > > > On Wed, May 20, 2020 at 06:06:50PM +0000, Changming Liu wrote:
> > > > > > The char buffer buf, accepts user data which might be negative value
> and
> > > > > > the content is left shifted to form an unsigned integer.
> > > > > >
> > > > > > Since left shifting a negative value is undefined behavior, thus change
> > > > > > the char to u8 to fix this
> > > > > >
> > > > > > Signed-off-by: Changming Liu <liu.changm@northeastern.edu>
> > > > > > ---
> > > > > >  drivers/usb/misc/sisusbvga/sisusb.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/usb/misc/sisusbvga/sisusb.c
> > > > > b/drivers/usb/misc/sisusbvga/sisusb.c
> > > > > > index fc8a5da4a07c..0734e6dd9386 100644
> > > > > > --- a/drivers/usb/misc/sisusbvga/sisusb.c
> > > > > > +++ b/drivers/usb/misc/sisusbvga/sisusb.c
> > > > > > @@ -761,7 +761,7 @@ static int sisusb_write_mem_bulk(struct
> > > > > sisusb_usb_data *sisusb, u32 addr,
> > > > > >         u8   swap8, fromkern = kernbuffer ? 1 : 0;
> > > > > >         u16  swap16;
> > > > > >         u32  swap32, flag = (length >> 28) & 1;
> > > > > > -       char buf[4];
> > > > > > +       u8 buf[4];
> > > > >
> > > > > Do we also need to change the kernbuffer variable from char* to be u8*
> > > > > as the same time to solve the same potential issue?
> > > > >
> > > >
> > > > This is a very good point, sorry I didn't notice this.
> > > > Indeed, according to the caller of sisusb_copy_memory, the wrapper of
> > > current function
> > > > there is no guarantee that each char in kernbuffer is positive.
> > > >
> > > > However, it seems if we change the function argument type directly from
> > > char* to u8*,
> > > > Other parts that call this function e.g. in sisusb_copy_memory
> > > > or uses this pointer e.g. line 770,line 883 must change accordingly.
> > > > Looks like many force casts which doesn't look too necessary.
> > > >
> > > > I wonder how about just force casting the content of kernbuffer when it's
> > > read in line 823 to line 829
> > > > from char to u8? This seems explicitly fix this bug.
> > >
> > > That will work, but how about just changing all instances of char to u8
> > > throughout this driver to make sure everything is working properly that
> > > way.  char should not be used as a type when copying around "raw" data
> > > like this from user-to-device for these reasons.
> > >
> >
> > This is a clean sweep, from the perspective of security I find no reason against
> it.
> > Indeed, u8 is strictly better than char when there is no need for any value to
> be negative.
> > I'd be very honored to see this through.
> >
> > I wonder, by this driver, you mean this sisusbvga module or something else?
> > Forgive me for my limited understanding of the module since I've only read
> the code related to this bug.
> > Please let me know on what files do you want to apply this change.
> 
> The sisusbvga module, all of the files that make it up, in
> drivers/usb/misc/sisusbvga/ are what I am referring to here.
> 
> > Or if you feel like doing this yourself please go ahead,
> > I'm still a bit daunted by the scale of changes that need to be made frankly :p
> 
> Nope, you can do this, it shouldn't be that hard.  Might take a few
> patches, do it as a patch series, doing one logical change per patch.
> 
> If you have specific questions, please let us know!
> 
Hi Greg,
Sorry for following up so late, I have been traveling for the past
 a few days.

I've received the emails from kbuild test robot about the 
sisbus_write and sisbus_read's declaration being incompilable
 with struct file_operations because I changed the buffer
from char* to u8* in my last patch.

Sorry I didn't know this would fail the test. Is there anything I 
could do to fix this? Should I submit another patch?

Best,
Changming

Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value

[Greg KH]

On Fri, Jun 05, 2020 at 10:13:13PM +0000, Changming Liu wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: Saturday, May 23, 2020 2:14 AM
> > To: Changming Liu <liu.changm@northeastern.edu>
> > Cc: thomas@winischhofer.net; linux-usb@vger.kernel.org
> > Subject: Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value
> > 
> > On Fri, May 22, 2020 at 07:14:32PM +0000, Changming Liu wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Greg KH <gregkh@linuxfoundation.org>
> > > > Sent: Friday, May 22, 2020 3:24 AM
> > > > To: Changming Liu <liu.changm@northeastern.edu>
> > > > Cc: thomas@winischhofer.net; linux-usb@vger.kernel.org
> > > > Subject: Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative
> > value
> > > >
> > > > On Thu, May 21, 2020 at 05:56:44PM +0000, Changming Liu wrote:
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Greg KH <gregkh@linuxfoundation.org>
> > > > > > Sent: Thursday, May 21, 2020 3:36 AM
> > > > > > To: Changming Liu <liu.changm@northeastern.edu>
> > > > > > Cc: thomas@winischhofer.net; linux-usb@vger.kernel.org
> > > > > > Subject: Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative
> > > > value
> > > > > >
> > > > > > On Wed, May 20, 2020 at 06:06:50PM +0000, Changming Liu wrote:
> > > > > > > The char buffer buf, accepts user data which might be negative value
> > and
> > > > > > > the content is left shifted to form an unsigned integer.
> > > > > > >
> > > > > > > Since left shifting a negative value is undefined behavior, thus change
> > > > > > > the char to u8 to fix this
> > > > > > >
> > > > > > > Signed-off-by: Changming Liu <liu.changm@northeastern.edu>
> > > > > > > ---
> > > > > > >  drivers/usb/misc/sisusbvga/sisusb.c | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/usb/misc/sisusbvga/sisusb.c
> > > > > > b/drivers/usb/misc/sisusbvga/sisusb.c
> > > > > > > index fc8a5da4a07c..0734e6dd9386 100644
> > > > > > > --- a/drivers/usb/misc/sisusbvga/sisusb.c
> > > > > > > +++ b/drivers/usb/misc/sisusbvga/sisusb.c
> > > > > > > @@ -761,7 +761,7 @@ static int sisusb_write_mem_bulk(struct
> > > > > > sisusb_usb_data *sisusb, u32 addr,
> > > > > > >         u8   swap8, fromkern = kernbuffer ? 1 : 0;
> > > > > > >         u16  swap16;
> > > > > > >         u32  swap32, flag = (length >> 28) & 1;
> > > > > > > -       char buf[4];
> > > > > > > +       u8 buf[4];
> > > > > >
> > > > > > Do we also need to change the kernbuffer variable from char* to be u8*
> > > > > > as the same time to solve the same potential issue?
> > > > > >
> > > > >
> > > > > This is a very good point, sorry I didn't notice this.
> > > > > Indeed, according to the caller of sisusb_copy_memory, the wrapper of
> > > > current function
> > > > > there is no guarantee that each char in kernbuffer is positive.
> > > > >
> > > > > However, it seems if we change the function argument type directly from
> > > > char* to u8*,
> > > > > Other parts that call this function e.g. in sisusb_copy_memory
> > > > > or uses this pointer e.g. line 770,line 883 must change accordingly.
> > > > > Looks like many force casts which doesn't look too necessary.
> > > > >
> > > > > I wonder how about just force casting the content of kernbuffer when it's
> > > > read in line 823 to line 829
> > > > > from char to u8? This seems explicitly fix this bug.
> > > >
> > > > That will work, but how about just changing all instances of char to u8
> > > > throughout this driver to make sure everything is working properly that
> > > > way.  char should not be used as a type when copying around "raw" data
> > > > like this from user-to-device for these reasons.
> > > >
> > >
> > > This is a clean sweep, from the perspective of security I find no reason against
> > it.
> > > Indeed, u8 is strictly better than char when there is no need for any value to
> > be negative.
> > > I'd be very honored to see this through.
> > >
> > > I wonder, by this driver, you mean this sisusbvga module or something else?
> > > Forgive me for my limited understanding of the module since I've only read
> > the code related to this bug.
> > > Please let me know on what files do you want to apply this change.
> > 
> > The sisusbvga module, all of the files that make it up, in
> > drivers/usb/misc/sisusbvga/ are what I am referring to here.
> > 
> > > Or if you feel like doing this yourself please go ahead,
> > > I'm still a bit daunted by the scale of changes that need to be made frankly :p
> > 
> > Nope, you can do this, it shouldn't be that hard.  Might take a few
> > patches, do it as a patch series, doing one logical change per patch.
> > 
> > If you have specific questions, please let us know!
> > 
> Hi Greg,
> Sorry for following up so late, I have been traveling for the past
>  a few days.
> 
> I've received the emails from kbuild test robot about the 
> sisbus_write and sisbus_read's declaration being incompilable
>  with struct file_operations because I changed the buffer
> from char* to u8* in my last patch.
> 
> Sorry I didn't know this would fail the test. Is there anything I 
> could do to fix this? Should I submit another patch?

Fix up the series and resend it.

thanks,

greg k-h

[PATCH] USB: sisusbvga: Fix left shifting a possible negative value

[Changming Liu]

> -----Original Message-----
> From: Greg KH <greg@kroah.com>
> Sent: Wednesday, May 20, 2020 1:02 AM
> To: Changming Liu <liu.changm@northeastern.edu>
> Cc: thomas@winischhofer.net; linux-usb@vger.kernel.org; Lu, Long
> <l.lu@northeastern.edu>; yaohway@gmail.com
> Subject: Re: [Bug Report] drivers/usb/misc/sisusbvga: undefined result when
> left shift a possible negative value in sisusb_write_mem_bulk
> 
> On Wed, May 20, 2020 at 03:51:04AM +0000, Changming Liu wrote:
> > Hi Greg and Thomas,
> > Greetings, I'm a first-year PhD student who is interested in the usage of
> UBSan for linux. And after some experiments, I've found that in
> drivers/usb/misc/sisusbvga/sisusb.c
> > function sisusb_write_mem_bulk, there is an undefined behavior caused by
> left shifting a possible negative number.
> >
> > More specifically, in the switch statement for case 3, after executing
> copy_from_user, the the lower 3 bytes of char buf[4] are filled with data from
> user space.
> > And these 3 bytes are left shifted accordingly to form a 32bit unsigned
> integer, swap32.
> >
> > The potential problem is, since the buf is declared as signed char buffer so
> each byte might be a negative number while being left shifted. According to the
> C standard, when the left-hand operand of the left shift operator is a negative
> value, the result is undefined. So I guess change the buf declaration to unsigned
> will help? Given that it's only used here.
> 
> Sounds like a good idea, patches are welcome to fix this.

Hi greg,
Thank you for this recognition! This means a lot to me. 
Here's the patch as we agreed.

Best,
Changming


From 14ae7c67ea3fb96ed6bea0bc9919f3c597308813 Mon Sep 17 00:00:00 2001
From: Changming Liu <liu.changm@northeastern.edu>
Date: Wed, 20 May 2020 12:19:37 -0400
Subject: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value

the char buffer buf, accepts user data which might be negative value and the content is left shifted to form an unsigned integer.
Since left shifting a negative value is undefined behavior, thus change the char to u8 to fix this

Signed-off-by: Changming Liu <liu.changm@northeastern.edu>
---
 drivers/usb/misc/sisusbvga/sisusb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/misc/sisusbvga/sisusb.c b/drivers/usb/misc/sisusbvga/sisusb.c
index fc8a5da4a07c..0734e6dd9386 100644
--- a/drivers/usb/misc/sisusbvga/sisusb.c
+++ b/drivers/usb/misc/sisusbvga/sisusb.c
@@ -761,7 +761,7 @@ static int sisusb_write_mem_bulk(struct sisusb_usb_data *sisusb, u32 addr,
        u8   swap8, fromkern = kernbuffer ? 1 : 0;
        u16  swap16;
        u32  swap32, flag = (length >> 28) & 1;
-       char buf[4];
+       u8 buf[4];

        /* if neither kernbuffer not userbuffer are given, assume
         * data in obuf
--
2.17.1

> thanks,
> 
> greg k-h

Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value

[Greg KH]

On Wed, May 20, 2020 at 04:37:50PM +0000, Changming Liu wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH <greg@kroah.com>
> > Sent: Wednesday, May 20, 2020 1:02 AM
> > To: Changming Liu <liu.changm@northeastern.edu>
> > Cc: thomas@winischhofer.net; linux-usb@vger.kernel.org; Lu, Long
> > <l.lu@northeastern.edu>; yaohway@gmail.com
> > Subject: Re: [Bug Report] drivers/usb/misc/sisusbvga: undefined result when
> > left shift a possible negative value in sisusb_write_mem_bulk
> > 
> > On Wed, May 20, 2020 at 03:51:04AM +0000, Changming Liu wrote:
> > > Hi Greg and Thomas,
> > > Greetings, I'm a first-year PhD student who is interested in the usage of
> > UBSan for linux. And after some experiments, I've found that in
> > drivers/usb/misc/sisusbvga/sisusb.c
> > > function sisusb_write_mem_bulk, there is an undefined behavior caused by
> > left shifting a possible negative number.
> > >
> > > More specifically, in the switch statement for case 3, after executing
> > copy_from_user, the the lower 3 bytes of char buf[4] are filled with data from
> > user space.
> > > And these 3 bytes are left shifted accordingly to form a 32bit unsigned
> > integer, swap32.
> > >
> > > The potential problem is, since the buf is declared as signed char buffer so
> > each byte might be a negative number while being left shifted. According to the
> > C standard, when the left-hand operand of the left shift operator is a negative
> > value, the result is undefined. So I guess change the buf declaration to unsigned
> > will help? Given that it's only used here.
> > 
> > Sounds like a good idea, patches are welcome to fix this.
> 
> Hi greg,
> Thank you for this recognition! This means a lot to me. 
> Here's the patch as we agreed.

Please resend this in a normal format where we can properly review it.

But:

> 
> Best,
> Changming
> 
> 
> >From 14ae7c67ea3fb96ed6bea0bc9919f3c597308813 Mon Sep 17 00:00:00 2001
> From: Changming Liu <liu.changm@northeastern.edu>
> Date: Wed, 20 May 2020 12:19:37 -0400
> Subject: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value
> 
> the char buffer buf, accepts user data which might be negative value and the content is left shifted to form an unsigned integer.
> Since left shifting a negative value is undefined behavior, thus change the char to u8 to fix this

Properly line-wrap your changelog when you resend this.

thanks,

greg k-h

RE: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value

[Changming Liu]

> -----Original Message-----
> From: Greg KH <greg@kroah.com>
> Sent: Wednesday, May 20, 2020 12:47 PM
> To: Changming Liu <liu.changm@northeastern.edu>
> Cc: thomas@winischhofer.net; linux-usb@vger.kernel.org; Lu, Long
> <l.lu@northeastern.edu>; yaohway@gmail.com
> Subject: Re: [PATCH] USB: sisusbvga: Fix left shifting a possible negative value
> 
> On Wed, May 20, 2020 at 04:37:50PM +0000, Changming Liu wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH <greg@kroah.com>
> > > Sent: Wednesday, May 20, 2020 1:02 AM
> > > To: Changming Liu <liu.changm@northeastern.edu>
> > > Cc: thomas@winischhofer.net; linux-usb@vger.kernel.org; Lu, Long
> > > <l.lu@northeastern.edu>; yaohway@gmail.com
> > > Subject: Re: [Bug Report] drivers/usb/misc/sisusbvga: undefined
> > > result when left shift a possible negative value in
> > > sisusb_write_mem_bulk
> > >
> > > On Wed, May 20, 2020 at 03:51:04AM +0000, Changming Liu wrote:
> > > > Hi Greg and Thomas,
> > > > Greetings, I'm a first-year PhD student who is interested in the
> > > > usage of
> > > UBSan for linux. And after some experiments, I've found that in
> > > drivers/usb/misc/sisusbvga/sisusb.c
> > > > function sisusb_write_mem_bulk, there is an undefined behavior
> > > > caused by
> > > left shifting a possible negative number.
> > > >
> > > > More specifically, in the switch statement for case 3, after
> > > > executing
> > > copy_from_user, the the lower 3 bytes of char buf[4] are filled with
> > > data from user space.
> > > > And these 3 bytes are left shifted accordingly to form a 32bit
> > > > unsigned
> > > integer, swap32.
> > > >
> > > > The potential problem is, since the buf is declared as signed char
> > > > buffer so
> > > each byte might be a negative number while being left shifted.
> > > According to the C standard, when the left-hand operand of the left
> > > shift operator is a negative value, the result is undefined. So I
> > > guess change the buf declaration to unsigned will help? Given that it's only
> used here.
> > >
> > > Sounds like a good idea, patches are welcome to fix this.
> >
> > Hi greg,
> > Thank you for this recognition! This means a lot to me.
> > Here's the patch as we agreed.
> 
> Please resend this in a normal format where we can properly review it.
Sure, I'm so sorry for this inconvenience, I've sent you a separate patch , hope this works.
> 
> But:
> 
> >
> > Best,
> > Changming
> >
> >
> > >From 14ae7c67ea3fb96ed6bea0bc9919f3c597308813 Mon Sep 17 00:00:00
> > >2001
> > From: Changming Liu <liu.changm@northeastern.edu>
> > Date: Wed, 20 May 2020 12:19:37 -0400
> > Subject: [PATCH] USB: sisusbvga: Fix left shifting a possible negative
> > value
> >
> > the char buffer buf, accepts user data which might be negative value and the
> content is left shifted to form an unsigned integer.
> > Since left shifting a negative value is undefined behavior, thus
> > change the char to u8 to fix this
> 
> Properly line-wrap your changelog when you resend this.
[Changming Liu] 
Got it, I believe in my patch, I properly line-wrapped the line, I hope the patch you received is well-formatted. 
Otherwise, I'll have to send through my gmail account.

Sorry again for this inconvenience.

Best,
Changming