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

Message

Changming June 19, 2020, 12:28 a.m. UTC

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

Comments

Greg KH June 19, 2020, 7 a.m. UTC | #1

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

Changming Liu June 19, 2020, 8:50 p.m. UTC | #2

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

Greg KH June 24, 2020, 3:13 p.m. UTC | #3

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