> -----Original Message-----
> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Saturday, June 27, 2020 7:28 AM
> To: Changming Liu <charley.ashbringer@xxxxxxxxx>
> Cc: linux-usb@xxxxxxxxxxxxxxx; thomas@xxxxxxxxxxxxxxxx
> 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@xxxxxxxxx>
> > ---
> > 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
