[PATCH] fs: fat: check for buffer size before reading blocks

[Heinrich Schuchardt]

On 9/16/21 4:09 PM, Oleksandr Suvorov wrote:
> From: Ricardo Salveti <ricardo at foundries.io>
>
> This patch optimizes the commit mentioned below by avoiding running
> a set of commands which useless in the case when

%s/which/which are/

> size < mydata->sect_size and idx would be 0.

Thank you for reviewing the FAT coding.

>
> Fixes: 5b3ddb17ba ("fs/fat/fat.c: Do not perform zero block reads if there are no blocks left")
>
> Signed-off-by: Ricardo Salveti <ricardo at foundries.io>
> Co-developed-by: Oleksandr Suvorov <oleksandr.suvorov at foundries.io>
> Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov at foundries.io>
> ---
>
>   fs/fat/fat.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index 7021138b987..4a509755442 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -275,14 +275,10 @@ get_cluster(fsdata *mydata, __u32 clustnum, __u8 *buffer, unsigned long size)
>   			buffer += mydata->sect_size;
>   			size -= mydata->sect_size;
>   		}
> -	} else {
> +	} else if (size >= mydata->sect_size) {
>   		__u32 idx;

The variable name idx is a misnomer. Can we call it sect_count?

> -
>   		idx = size / mydata->sect_size;
> -		if (idx == 0)
> -			ret = 0;

size could be 0. In this case we still have to set ret = 0. Both for the
DMA aligned and the unaligned case. You could initialize ret = 0 where
it is defined.

> -		else
> -			ret = disk_read(startsect, idx, buffer);
> +		ret = disk_read(startsect, idx, buffer);
>   		if (ret != idx) {
>   			debug("Error reading data (got %d)\n", ret);
>   			return -1;
>

  291                 idx *= mydata->sect_size;
  292                 buffer += idx;
  293                 size -= idx;

These lines should be adjusted, too: size is defined as unsigned long.
We should not use the u32 variable idx but a separate variable of type
unsigned long to hold the number of bytes read.

Best regards

Heinrich