[PATCH 1/1] fs/squashfs: parameter check sqfs_read_metablock()

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/1] fs/squashfs: parameter check sqfs_read_metablock()

Heinrich Schuchardt
We should check if the incoming parameter file_mapping is not NULL instead
of checking after adding an offset.

Reported-by: Coverity CID 307210
Signed-off-by: Heinrich Schuchardt <[hidden email]>
---
 fs/squashfs/sqfs_inode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/squashfs/sqfs_inode.c b/fs/squashfs/sqfs_inode.c
index 1368f3063c..14d70cf678 100644
--- a/fs/squashfs/sqfs_inode.c
+++ b/fs/squashfs/sqfs_inode.c
@@ -141,9 +141,9 @@ int sqfs_read_metablock(unsigned char *file_mapping, int offset,
  const unsigned char *data;
  u16 header;

- data = file_mapping + offset;
- if (!data)
+ if (!file_mapping)
  return -EFAULT;
+ data = file_mapping + offset;

  header = get_unaligned((u16 *)data);
  if (!header)
--
2.28.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] fs/squashfs: parameter check sqfs_read_metablock()

Thomas Petazzoni
On Wed, 23 Sep 2020 21:13:51 +0200
Heinrich Schuchardt <[hidden email]> wrote:

> We should check if the incoming parameter file_mapping is not NULL instead
> of checking after adding an offset.
>
> Reported-by: Coverity CID 307210
> Signed-off-by: Heinrich Schuchardt <[hidden email]>

Acked-by: Thomas Petazzoni <[hidden email]>

However, I wonder if this check is really useful. sqfs_read_metablock()
is an internal function, so it should be up to the callers to make sure
that they don't pass a NULL file_mapping argument.

Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] fs/squashfs: parameter check sqfs_read_metablock()

Heinrich Schuchardt
On 9/23/20 9:49 PM, Thomas Petazzoni wrote:

> On Wed, 23 Sep 2020 21:13:51 +0200
> Heinrich Schuchardt <[hidden email]> wrote:
>
>> We should check if the incoming parameter file_mapping is not NULL instead
>> of checking after adding an offset.
>>
>> Reported-by: Coverity CID 307210
>> Signed-off-by: Heinrich Schuchardt <[hidden email]>
>
> Acked-by: Thomas Petazzoni <[hidden email]>
>
> However, I wonder if this check is really useful. sqfs_read_metablock()
> is an internal function, so it should be up to the callers to make sure
> that they don't pass a NULL file_mapping argument.
>
> Thomas
>

This is a question of programming style. Eliminating the check may be
justified if the input variable is checked by every caller.

The real problems are elsewhere, e.g.

sqfs_search_dir() and sqfs_readdir() do not check the return value of
sqfs_find_inode() which may be NULL.

sqfs_opendir() leaks allocated memory if an error occurs.

Best regards

Heinrich
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/1] fs/squashfs: parameter check sqfs_read_metablock()

Tom Rini-4
In reply to this post by Heinrich Schuchardt
On Wed, Sep 23, 2020 at 09:13:51PM +0200, Heinrich Schuchardt wrote:

> We should check if the incoming parameter file_mapping is not NULL instead
> of checking after adding an offset.
>
> Reported-by: Coverity CID 307210
> Signed-off-by: Heinrich Schuchardt <[hidden email]>
> Acked-by: Thomas Petazzoni <[hidden email]>

Applied to u-boot/master, thanks!

--
Tom

signature.asc (673 bytes) Download Attachment