| Message ID | 1593200057-245-2-git-send-email-charley.ashbringer@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: sisusbvga: cleaning up char buffers to u8 buffer | expand |
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. > > 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. > 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 change things that actually matter, and the above api changes don't change anything at all, right? thanks, greg k-h
> -----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
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, ssize_t *bytes_written, unsigned int tflags, int async) { int result = 0, retry, count = len; @@ -543,7 +543,7 @@ static int sisusb_send_packet(struct sisusb_usb_data *sisusb, int len, /* 1. send the packet */ ret = sisusb_send_bulk_msg(sisusb, SISUSB_EP_GFX_OUT, len, - (char *)packet, NULL, 0, &bytes_transferred, 0, 0); + (u8 *)packet, NULL, 0, &bytes_transferred, 0, 0); if ((ret == 0) && (len == 6)) { @@ -579,7 +579,7 @@ static int sisusb_send_bridge_packet(struct sisusb_usb_data *sisusb, int len, /* 1. send the packet */ ret = sisusb_send_bulk_msg(sisusb, SISUSB_EP_BRIDGE_OUT, len, - (char *)packet, NULL, 0, &bytes_transferred, tflags, 0); + (u8 *)packet, NULL, 0, &bytes_transferred, tflags, 0); if ((ret == 0) && (len == 6)) { @@ -752,7 +752,7 @@ static int sisusb_write_memio_long(struct sisusb_usb_data *sisusb, int type, */ 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, 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]; /* if neither kernbuffer not userbuffer are given, assume * data in obuf @@ -2700,7 +2700,7 @@ static ssize_t sisusb_write(struct file *file, const char __user *buffer, * mode or if YUV data is being transferred). */ errno = sisusb_write_mem_bulk(sisusb, address, NULL, - count, buffer, 0, &bytes_written); + count, (u8 __user *)buffer, 0, &bytes_written); if (bytes_written) errno = bytes_written; @@ -2718,7 +2718,7 @@ static ssize_t sisusb_write(struct file *file, const char __user *buffer, * in advance. */ errno = sisusb_write_mem_bulk(sisusb, address, NULL, - count, buffer, 0, &bytes_written); + count, (u8 __user *)buffer, 0, &bytes_written); if (bytes_written) errno = bytes_written;
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. Signed-off-by: Changming Liu <charley.ashbringer@gmail.com> --- drivers/usb/misc/sisusbvga/sisusb.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)