[PATCH 0/4] USB: sisusbvga: cleaning up char buffers to u8 buffer

[PATCH 0/4] USB: sisusbvga: cleaning up char buffers to u8 buffer

[Changming Liu]

The purpose of this patch series is to prevent
undefined behavior caused by arithemetic operations on
the content of the char buffer.This is achieved through
converting all char buffers to u8 when there is 
no need for the content to be negative value(which is the 
case for all touched buffer in this patch series).

Converting these char buffers to u8 buffers will make 
certain types of UB from producing undefined result to 
well-defined intended (modular) result, and will not 
introduce any unintended side effect.

As an example, changing the local char buf[4] in sisusb_write_mem_bulk
to u8 fixes an UB: since buf is filled with data from user space,
thus can be negative. Left-shifting this negative value produces
undefined result, this can be fixed by changing it from char 
to u8.

This however doesn't apply to ioctl interfaces functions, since the 
types for buffer of ioctl-like functions need to be 
char* instead of u8* for compatibility reason.
In these cases, the buffer declared as char* will be force cast
to u8* when being used.


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 the char buffer from char to u8 for sisusb_write_mem_bulk and sisusb_send_bulk_msg

[Changming Liu]

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(-)

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 the char buffer from char to u8 for sisusb_write_mem_bulk and sisusb_send_bulk_msg

[Greg KH]

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

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]

> -----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

[PATCH 2/4] USB: sisusbvga: change the buffer members in sisusb_usb_data from char to u8

[Changming Liu]

This patch changes the buffer within struct sisusb_usb_data, 
namely, ibuf,obuf and font_backup
from char* to unsigned char* to avoid any related UB.

Thia patch also changes the buffer declared in the code that gets 
assigned to by these buffers from char to u8 as well.

Signed-off-by: Changming Liu <charley.ashbringer@gmail.com>
---
 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

Re: [PATCH 2/4] USB: sisusbvga: change the buffer members in sisusb_usb_data from char to u8

[Greg KH]

On Fri, Jun 26, 2020 at 03:34:15PM -0400, Changming Liu wrote:
> This patch changes the buffer within struct sisusb_usb_data, 
> namely, ibuf,obuf and font_backup
> from char* to unsigned char* to avoid any related UB.
> 
> Thia patch also changes the buffer declared in the code that gets 
> assigned to by these buffers from char to u8 as well.
> 
> Signed-off-by: Changming Liu <charley.ashbringer@gmail.com>
> ---
>  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 */

u8 for this?

>  	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;

u8 like you say you are changing in the above text?

thanks,

greg k-h

[PATCH 3/4] USB: sisusbvga: change the buffer in sisusb_recv_bulk_msg from char to u8

[Changming Liu]

This patch changes the userbuffer of sisusb_recv_bulk_msg from char to u8
to avoid related UB.

Also, for kernbuffer declared as void* in the function header, force cast 
the passed-in parameters from char* to u8*.

Signed-off-by: Changming Liu <charley.ashbringer@gmail.com>
---
 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

Re: [PATCH 3/4] USB: sisusbvga: change the buffer in sisusb_recv_bulk_msg from char to u8

[Greg KH]

On Fri, Jun 26, 2020 at 03:34:16PM -0400, Changming Liu wrote:
> This patch changes the userbuffer of sisusb_recv_bulk_msg from char to u8
> to avoid related UB.
> 
> Also, for kernbuffer declared as void* in the function header, force cast 
> the passed-in parameters from char* to u8*.
> 
> Signed-off-by: Changming Liu <charley.ashbringer@gmail.com>
> ---
>  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,

Same as before, I do not think you need to change these, do you?

thanks,

greg k-h

[PATCH 4/4] USB: sisusbvga: change the buffers in sisusb_read_mem_bulk from char to u8

[Changming Liu]

This patch changes the userbuffer,kernbuffer in the header of 
sisusb_read_mem_bulk from char to u8 to avoid related UB.

Also a local buffer, buf, in sisusb_read_mem_bulk is changed
from char to u8.

Since sisusb_read_mem_bulk is called by sisusb_read_memory 
sisusb_testreadwrite and sisusb_read, change their 
passed-in parameters types from char to u8 accordingly.

Signed-off-by: Changming Liu <charley.ashbringer@gmail.com>
---
 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