RE: [PATCH 1/4] USB: sisusbvga: change the char buffer from char to u8 for sisusb_write_mem_bulk and sisusb_send_bulk_msg

[<charley.ashbringer@gmail.com>]

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Saturday, June 27, 2020 7:28 AM
> To: Changming Liu <charley.ashbringer@gmail.com>
> Cc: linux-usb@vger.kernel.org; thomas@winischhofer.net
> Subject: Re: [PATCH 1/4] USB: sisusbvga: change the char buffer from char
to
> u8 for sisusb_write_mem_bulk and sisusb_send_bulk_msg
> 
> On Fri, Jun 26, 2020 at 03:34:14PM -0400, Changming Liu wrote:
> > This patch changes the types of char buffer declarations
> > as well as passed-in parameters to u8 for the function
> > sisusb_write_mem_bulk and sisusb_send_bulk_msg to aviod
> > any related UB.
> >
> > This patch also change the local buf[4] of sisusb_write_mem_bulk
> > to u8. This fixed an undefined behavior, since buf can be filled
> > with data from user space, thus can be negative given it's signed,
> > and its content is being left-shifted. Left-shifting a negative
> > value is undefined behavior. It's fixed by changing the buf from
> > char to u8.
> 
> In looking at this closer, it doesn't make sense to change the function
> parameters here, as everything that deals with the pointer already
> handles the change properly.
> 

Quite,  no security issue could possibly be raised without 
these unnecessary changes. 

> 
> >
> > Signed-off-by: Changming Liu <charley.ashbringer@gmail.com>
> > ---
> >  drivers/usb/misc/sisusbvga/sisusb.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/misc/sisusbvga/sisusb.c
> b/drivers/usb/misc/sisusbvga/sisusb.c
> > index fc8a5da..4aa717a 100644
> > --- a/drivers/usb/misc/sisusbvga/sisusb.c
> > +++ b/drivers/usb/misc/sisusbvga/sisusb.c
> > @@ -327,7 +327,7 @@ static int sisusb_bulkin_msg(struct sisusb_usb_data
> *sisusb,
> >   */
> >
> >  static int sisusb_send_bulk_msg(struct sisusb_usb_data *sisusb, int ep,
int
> len,
> > -		char *kernbuffer, const char __user *userbuffer, int index,
> > +		u8 *kernbuffer, const u8 __user *userbuffer, int index,
> 
> So the kernbuffer pointer might want to be changed, but in looking at
> the code, there's no difference here, it can be left alone.
> 
> The userbuffer does not need to be changed at all.
> 
> >  static int sisusb_write_mem_bulk(struct sisusb_usb_data *sisusb, u32
addr,
> > -		char *kernbuffer, int length, const char __user *userbuffer,
> > +		u8 *kernbuffer, int length, const u8 __user *userbuffer,
> 
> Same here, these do not need to be changed.

Totally agree.

> 
> >  		int index, ssize_t *bytes_written)
> >  {
> >  	struct sisusb_packet packet;
> > @@ -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];
> 
> That is what should be changed, and in looking at the code path, I think
> that's it here.
> 
> Sorry for taking you down the wrong path, but I think you should only


It's totally fine, I took this chance to thoroughly read the code 
and learned a lot about how a typical linux driver is written : p


> change things that actually matter, and the above api changes don't
> change anything at all, right?

Yes, this is exactly what I felt when I was compiling the chances.
I really don't see necessity in the changes except the one
that has security implication.

Thanks for the feedback, these back-and-forth deepen my understanding
both of the kernel and how to submit patch.

Sorry for this late reply, I have been catching a deadline for the 
past several days :( I'll submit another patch about the change with
security implication shortly.

Best regards,
Changming