| Message ID | 20200923191351.33474-1-xypron.glpk@gmx.de |
|---|---|
| State | Accepted |
| Commit | 0ed375ebb13f8b2d69400b9df8985c8123d3fbb1 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [1/1] fs/squashfs: parameter check sqfs_read_metablock() | expand |
On Wed, 23 Sep 2020 21:13:51 +0200 Heinrich Schuchardt <xypron.glpk@gmx.de> 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 <xypron.glpk@gmx.de> Acked-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> 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
On 9/23/20 9:49 PM, Thomas Petazzoni wrote: > On Wed, 23 Sep 2020 21:13:51 +0200 > Heinrich Schuchardt <xypron.glpk@gmx.de> 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 <xypron.glpk@gmx.de> > > Acked-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > > 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
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 <xypron.glpk@gmx.de> > Acked-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> Applied to u-boot/master, thanks!
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)
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 <xypron.glpk@gmx.de> --- fs/squashfs/sqfs_inode.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 2.28.0