[PATCH 0/4] USB: sisusbvga: series of changes char to u8

[PATCH 0/4] USB: sisusbvga: series of changes char to u8

[Changming Liu]

This patch series changes all appropriate instances
of signed char arrays or buffer to unsigned char.

For each patch, if changing one variable to u8
involves its callers/callees, then those changes
are included in that patch as well.

This doesn't apply to ioctl functions, since 
the types for buffer of ioctl-like functions
needs to be char* instead of u8* to keep
the compiler happy.


Changming Liu (4):
  USB: sisusbvga: change sisusb_write_mem_bulk
  USB: sisusbvga: change the buffers of sisusb from char to u8
  USB: sisusbvga: change userbuffer for sisusb_recv_bulk_msg to u8
  USB: sisusbvga: change sisusb_read_mem_bulk

 drivers/usb/misc/sisusbvga/sisusb.c | 34 +++++++++++++++++-----------------
 drivers/usb/misc/sisusbvga/sisusb.h |  4 ++--
 2 files changed, 19 insertions(+), 19 deletions(-)

-- 
2.7.4

[PATCH 1/4] USB: sisusbvga: change sisusb_write_mem_bulk

[Changming Liu]

isusb_write_mem_bulk calls sisusb_send_bulk_msg and
is called by sisusb_write.
Changed their parameters accordingly.

Also change the local buf[4] of sisusb_write_mem_bulk
to u8. This fixed a potential undefined behavior.

Signed-off-by: Changming Liu <liu.changm@northeastern.edu>
---
 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,
 		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;
-- 
2.7.4

Re: [PATCH 1/4] USB: sisusbvga: change sisusb_write_mem_bulk

[Greg KH]

On Thu, Jun 18, 2020 at 08:28:51PM -0400, Changming Liu wrote:
> isusb_write_mem_bulk calls sisusb_send_bulk_msg and
> is called by sisusb_write.
> Changed their parameters accordingly.

Accordingly to what?  The subject just says "change", it does not say
what any of this was changed to here.

> 
> Also change the local buf[4] of sisusb_write_mem_bulk
> to u8. This fixed a potential undefined behavior.

Shouldn't this be a separate patch so that I could be backported to
older kernels as it would fix a potential issue?

When you have "also" in a patch description, it almost always means you
should split the patch up into multiple changes.

> 
> Signed-off-by: Changming Liu <liu.changm@northeastern.edu>

This email address doesn't match the "From:" line :(

thanks,

greg k-h

[PATCH 2/4] USB: sisusbvga: change the buffers of sisusb from char to u8

[Changming Liu]

change the buffer of sisusb namely, ibuf,obuf,
font_backup from char* to unsigned char*.
Also change the related buffer which can be assigned
to them

Signed-off-by: Changming Liu <liu.changm@northeastern.edu>
---
 drivers/usb/misc/sisusbvga/sisusb.c | 4 ++--
 drivers/usb/misc/sisusbvga/sisusb.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/misc/sisusbvga/sisusb.c b/drivers/usb/misc/sisusbvga/sisusb.c
index 4aa717a..8878c28 100644
--- a/drivers/usb/misc/sisusbvga/sisusb.c
+++ b/drivers/usb/misc/sisusbvga/sisusb.c
@@ -335,7 +335,7 @@ static int sisusb_send_bulk_msg(struct sisusb_usb_data *sisusb, int ep, int len,
 	int fromuser = (userbuffer != NULL) ? 1 : 0;
 	int fromkern = (kernbuffer != NULL) ? 1 : 0;
 	unsigned int pipe;
-	char *buffer;
+	u8 *buffer;
 
 	(*bytes_written) = 0;
 
@@ -454,7 +454,7 @@ static int sisusb_recv_bulk_msg(struct sisusb_usb_data *sisusb, int ep, int len,
 	int result = 0, retry, count = len;
 	int bufsize, thispass, transferred_len;
 	unsigned int pipe;
-	char *buffer;
+	u8 *buffer;
 
 	(*bytes_read) = 0;
 
diff --git a/drivers/usb/misc/sisusbvga/sisusb.h b/drivers/usb/misc/sisusbvga/sisusb.h
index c0fb9e1..8fe5d07 100644
--- a/drivers/usb/misc/sisusbvga/sisusb.h
+++ b/drivers/usb/misc/sisusbvga/sisusb.h
@@ -109,7 +109,7 @@ struct sisusb_usb_data {
 	int present;		/* !=0 if device is present on the bus */
 	int ready;		/* !=0 if device is ready for userland */
 	int numobufs;		/* number of obufs = number of out urbs */
-	char *obuf[NUMOBUFS], *ibuf;	/* transfer buffers */
+	unsigned char *obuf[NUMOBUFS], *ibuf;	/* transfer buffers */
 	int obufsize, ibufsize;
 	struct urb *sisurbout[NUMOBUFS];
 	struct urb *sisurbin;
@@ -140,7 +140,7 @@ struct sisusb_usb_data {
 	int sisusb_cursor_size_to;
 	int current_font_height, current_font_512;
 	int font_backup_size, font_backup_height, font_backup_512;
-	char *font_backup;
+	unsigned char *font_backup;
 	int font_slot;
 	struct vc_data *sisusb_display_fg;
 	int is_gfx;
-- 
2.7.4

[PATCH 3/4] USB: sisusbvga: change userbuffer for sisusb_recv_bulk_msg to u8

[Changming Liu]

change the userbuffer of sisusb_recv_bulk_msg from char to u8
also force cast the kernbuffer to u8 when passed in

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

diff --git a/drivers/usb/misc/sisusbvga/sisusb.c b/drivers/usb/misc/sisusbvga/sisusb.c
index 8878c28..86638c1 100644
--- a/drivers/usb/misc/sisusbvga/sisusb.c
+++ b/drivers/usb/misc/sisusbvga/sisusb.c
@@ -448,7 +448,7 @@ static int sisusb_send_bulk_msg(struct sisusb_usb_data *sisusb, int ep, int len,
  */
 
 static int sisusb_recv_bulk_msg(struct sisusb_usb_data *sisusb, int ep, int len,
-		void *kernbuffer, char __user *userbuffer, ssize_t *bytes_read,
+		void *kernbuffer, u8 __user *userbuffer, ssize_t *bytes_read,
 		unsigned int tflags)
 {
 	int result = 0, retry, count = len;
@@ -551,7 +551,7 @@ static int sisusb_send_packet(struct sisusb_usb_data *sisusb, int len,
 		 *    return value and write it to packet->data
 		 */
 		ret = sisusb_recv_bulk_msg(sisusb, SISUSB_EP_GFX_IN, 4,
-				(char *)&tmp, NULL, &bytes_transferred, 0);
+				(u8 *)&tmp, NULL, &bytes_transferred, 0);
 
 		packet->data = le32_to_cpu(tmp);
 	}
@@ -587,7 +587,7 @@ static int sisusb_send_bridge_packet(struct sisusb_usb_data *sisusb, int len,
 		 *    return value and write it to packet->data
 		 */
 		ret = sisusb_recv_bulk_msg(sisusb, SISUSB_EP_BRIDGE_IN, 4,
-				(char *)&tmp, NULL, &bytes_transferred, 0);
+				(u8 *)&tmp, NULL, &bytes_transferred, 0);
 
 		packet->data = le32_to_cpu(tmp);
 	}
-- 
2.7.4

[PATCH 4/4] USB: sisusbvga: change sisusb_read_mem_bulk

[Changming Liu]

change sisusb_read_mem_bulk userbuffer,kernbuffer
and local buf from char to u8.

Also it's called by sisusb_read_memory sisusb_testreadwrite
and sisusb_read. Change their parameter types accordingly.

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

diff --git a/drivers/usb/misc/sisusbvga/sisusb.c b/drivers/usb/misc/sisusbvga/sisusb.c
index 86638c1..f1b46b5 100644
--- a/drivers/usb/misc/sisusbvga/sisusb.c
+++ b/drivers/usb/misc/sisusbvga/sisusb.c
@@ -1108,7 +1108,7 @@ static int sisusb_read_mem_bulk(struct sisusb_usb_data *sisusb, u32 addr,
 		ssize_t *bytes_read)
 {
 	int ret = 0;
-	char buf[4];
+	u8 buf[4];
 	u16 swap16;
 	u32 swap32;
 
@@ -1293,7 +1293,7 @@ int sisusb_copy_memory(struct sisusb_usb_data *sisusb, char *src,
 }
 
 #ifdef SISUSBENDIANTEST
-static int sisusb_read_memory(struct sisusb_usb_data *sisusb, char *dest,
+static int sisusb_read_memory(struct sisusb_usb_data *sisusb, u8 *dest,
 		u32 src, int length)
 {
 	size_t dummy;
@@ -1308,7 +1308,7 @@ static int sisusb_read_memory(struct sisusb_usb_data *sisusb, char *dest,
 static void sisusb_testreadwrite(struct sisusb_usb_data *sisusb)
 {
 	static char srcbuffer[] = { 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77 };
-	char destbuffer[10];
+	u8 destbuffer[10];
 	int i, j;
 
 	sisusb_copy_memory(sisusb, srcbuffer, sisusb->vrambase, 7);
@@ -2561,7 +2561,7 @@ static ssize_t sisusb_read(struct file *file, char __user *buffer,
 		 * Remember: Data delivered is never endian-corrected
 		 */
 		errno = sisusb_read_mem_bulk(sisusb, address,
-				NULL, count, buffer, &bytes_read);
+				NULL, count, (u8 __user *)buffer, &bytes_read);
 
 		if (bytes_read)
 			errno = bytes_read;
@@ -2577,7 +2577,7 @@ static ssize_t sisusb_read(struct file *file, char __user *buffer,
 		 * Remember: Data delivered is never endian-corrected
 		 */
 		errno = sisusb_read_mem_bulk(sisusb, address,
-				NULL, count, buffer, &bytes_read);
+				NULL, count, (u8 __user *)buffer, &bytes_read);
 
 		if (bytes_read)
 			errno = bytes_read;
-- 
2.7.4

Re: [PATCH 0/4] USB: sisusbvga: series of changes char to u8

[Greg KH]

On Thu, Jun 18, 2020 at 08:28:50PM -0400, Changming Liu wrote:
> This patch series changes all appropriate instances
> of signed char arrays or buffer to unsigned char.
> 
> For each patch, if changing one variable to u8
> involves its callers/callees, then those changes
> are included in that patch as well.
> 
> This doesn't apply to ioctl functions, since 
> the types for buffer of ioctl-like functions
> needs to be char* instead of u8* to keep
> the compiler happy.

Why is that?  What is forcing those types to be that way?  These are all
self-contained in the driver itself, so they should be able to be
changed, right?

Do you have an example of a function that you want to change but somehow
can not?

thanks,

greg k-h

RE: [PATCH 0/4] USB: sisusbvga: series of changes char to u8

[Changming Liu]

> > This patch series changes all appropriate instances of signed char
> > arrays or buffer to unsigned char.
> >
> > For each patch, if changing one variable to u8 involves its
> > callers/callees, then those changes are included in that patch as
> > well.
> >
> > This doesn't apply to ioctl functions, since the types for buffer of
> > ioctl-like functions needs to be char* instead of u8* to keep the
> > compiler happy.
> 
> Why is that?  What is forcing those types to be that way?  These are all self-
> contained in the driver itself, so they should be able to be changed, right?
> 
> Do you have an example of a function that you want to change but somehow
> can not?
> 
Sorry for this confusion, I should have put more context into this patch.
This is a re-send of a former patch which was rejected by kernel build
test robot when I tried to change all char instances of this driver to
u8 in order to remove any potential undefined behaviors.

This patch(also the former rejected one) were based on a former discussion
with you, the email was quite lengthy, so I attached the link here for 
your reference. https://www.spinics.net/lists/linux-usb/msg196153.html

In conclusion, only the one I noted in the link has security implication 
and should be fixed, the other changes from char to u8 are just 
"in case".

If you still think it's needed to change all instances 
of char in this driver to u8, I'll enrich the patch note(which I should 
have done earlier) and re-send the patch series again.
Or if you think just fixing that specific UB in sisusb_write_mem_bulk
is enough, I'll submit another patch.

Sorry again for this lost of context and the inconvenience.
Best,
Changming

Re: [PATCH 0/4] USB: sisusbvga: series of changes char to u8

[Greg KH]

On Fri, Jun 19, 2020 at 08:50:52PM +0000, Changming Liu wrote:
> > > This patch series changes all appropriate instances of signed char
> > > arrays or buffer to unsigned char.
> > >
> > > For each patch, if changing one variable to u8 involves its
> > > callers/callees, then those changes are included in that patch as
> > > well.
> > >
> > > This doesn't apply to ioctl functions, since the types for buffer of
> > > ioctl-like functions needs to be char* instead of u8* to keep the
> > > compiler happy.
> > 
> > Why is that?  What is forcing those types to be that way?  These are all self-
> > contained in the driver itself, so they should be able to be changed, right?
> > 
> > Do you have an example of a function that you want to change but somehow
> > can not?
> > 
> Sorry for this confusion, I should have put more context into this patch.
> This is a re-send of a former patch which was rejected by kernel build
> test robot when I tried to change all char instances of this driver to
> u8 in order to remove any potential undefined behaviors.
> 
> This patch(also the former rejected one) were based on a former discussion
> with you, the email was quite lengthy, so I attached the link here for 
> your reference. https://www.spinics.net/lists/linux-usb/msg196153.html
> 
> In conclusion, only the one I noted in the link has security implication 
> and should be fixed, the other changes from char to u8 are just 
> "in case".
> 
> If you still think it's needed to change all instances 
> of char in this driver to u8, I'll enrich the patch note(which I should 
> have done earlier) and re-send the patch series again.
> Or if you think just fixing that specific UB in sisusb_write_mem_bulk
> is enough, I'll submit another patch.

I think cleaning up everything is good, so fixing that up and resending
it would be great to have.

thanks,

greg k-h