Re: [PATCH v2 2/2] staging: gs_fpgaboot: change char to u8

[Joe Perches]

On Mon, 2017-07-17 at 20:47 -0400, Jacob von Chorus wrote:
> The bitstream storage variables were changed from char to u8 arrays to
> prevent issues such as negative lengths. This change makes the code
> compatible with the "data" field in "struct firmware" which is of type
> u8.
> 
> Signed-off-by: Jacob von Chorus <jacobvonchorus@xxxxxxxxxx>
> ---
>  drivers/staging/gs_fpgaboot/gs_fpgaboot.c | 24 ++++++++++++------------
>  drivers/staging/gs_fpgaboot/gs_fpgaboot.h |  2 +-
>  2 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/gs_fpgaboot/gs_fpgaboot.c b/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
> index 008ef99f05..467a0ad81f 100644
> --- a/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
> +++ b/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
> @@ -41,16 +41,16 @@ static char	*file = "xlinx_fpga_firmware.bit";
>  module_param(file, charp, 0444);
>  MODULE_PARM_DESC(file, "Xilinx FPGA firmware file.");
>  
> -static void read_bitstream(char *bitdata, char *buf, int *offset, int rdsize)
> +static void read_bitstream(u8 *bitdata, u8 *buf, int *offset, int rdsize)
>  {
>  	memcpy(buf, bitdata + *offset, rdsize);
>  	*offset += rdsize;
>  }
>  
> -static int readinfo_bitstream(char *bitdata, char *buf, int size, int *offset)
> +static int readinfo_bitstream(u8 *bitdata, u8 *buf, int size, int *offset)
>  {
> -	char tbuf[64];
> -	s32 len;
> +	u8 tbuf[64];
> +	u16 len;
>  
>  	/* read section char */
>  	read_bitstream(bitdata, tbuf, offset, 1);

read_bitstream takes an int rdsize, not a u16.
and this function will overflow tbuf if len > 64

static void readinfo_bitstream(char *bitdata, char *buf, int *offset)
{
	char tbuf[64];
	s32 len;

	/* read section char */
	read_bitstream(bitdata, tbuf, offset, 1);

	/* read length */
	read_bitstream(bitdata, tbuf, offset, 2);

	len = tbuf[0] << 8 | tbuf[1];

	read_bitstream(bitdata, buf, offset, len);
	buf[len] = '\0';
}

len is up to 64k but tbuf is 64 bytes.

	len = get_unaligned_le16(tbuf)

might be nicer than

	len = tbuf[0] << 8 | tbuf[1];