ovl: avoid possible NULL dereference

[PATCH] ovl: avoid possible NULL dereference

Posted by Su Hui 2 years, 4 months ago

smatch warn:
fs/overlayfs/copy_up.c:450 ovl_set_origin() warn:
variable dereferenced before check 'fh' (see line 449)

If 'fh' is NULL, passing NULL instead of 'fh->buf'.

Signed-off-by: Su Hui <suhui@nfschina.com>
---
 fs/overlayfs/copy_up.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index d1761ec5866a..086f9176b4d4 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -446,7 +446,7 @@ int ovl_set_origin(struct ovl_fs *ofs, struct dentry *lower,
 	/*
 	 * Do not fail when upper doesn't support xattrs.
 	 */
-	err = ovl_check_setxattr(ofs, upper, OVL_XATTR_ORIGIN, fh->buf,
+	err = ovl_check_setxattr(ofs, upper, OVL_XATTR_ORIGIN, fh ? fh->buf : NULL,
 				 fh ? fh->fb.len : 0, 0);
 	kfree(fh);
 
-- 
2.30.2

Re: [PATCH] ovl: avoid possible NULL dereference

Posted by Amir Goldstein 2 years, 4 months ago

On Mon, Sep 25, 2023 at 7:52 AM Su Hui <suhui@nfschina.com> wrote:
>
> smatch warn:
> fs/overlayfs/copy_up.c:450 ovl_set_origin() warn:
> variable dereferenced before check 'fh' (see line 449)
>
> If 'fh' is NULL, passing NULL instead of 'fh->buf'.
>
> Signed-off-by: Su Hui <suhui@nfschina.com>
> ---
>  fs/overlayfs/copy_up.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index d1761ec5866a..086f9176b4d4 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -446,7 +446,7 @@ int ovl_set_origin(struct ovl_fs *ofs, struct dentry *lower,
>         /*
>          * Do not fail when upper doesn't support xattrs.
>          */
> -       err = ovl_check_setxattr(ofs, upper, OVL_XATTR_ORIGIN, fh->buf,
> +       err = ovl_check_setxattr(ofs, upper, OVL_XATTR_ORIGIN, fh ? fh->buf : NULL,
>                                  fh ? fh->fb.len : 0, 0);
>         kfree(fh);
>
> --
> 2.30.2

After discussing this with Dan Carpenter, this is not a kernel bug,
it is a smatch bug.

The value being passed to setxattr is (void *)OVL_FH_FID_OFFSET,
which is just as arbitrary as NULL when size is 0.

Thanks,
Amir.

Re: [PATCH] ovl: avoid possible NULL dereference

Posted by Dan Carpenter 2 years, 4 months ago

On Wed, Sep 27, 2023 at 05:02:26PM +0300, Amir Goldstein wrote:
> On Mon, Sep 25, 2023 at 7:52 AM Su Hui <suhui@nfschina.com> wrote:
> >
> > smatch warn:
> > fs/overlayfs/copy_up.c:450 ovl_set_origin() warn:
> > variable dereferenced before check 'fh' (see line 449)
> >
> > If 'fh' is NULL, passing NULL instead of 'fh->buf'.
> >
> > Signed-off-by: Su Hui <suhui@nfschina.com>
> > ---
> >  fs/overlayfs/copy_up.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index d1761ec5866a..086f9176b4d4 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -446,7 +446,7 @@ int ovl_set_origin(struct ovl_fs *ofs, struct dentry *lower,
> >         /*
> >          * Do not fail when upper doesn't support xattrs.
> >          */
> > -       err = ovl_check_setxattr(ofs, upper, OVL_XATTR_ORIGIN, fh->buf,
> > +       err = ovl_check_setxattr(ofs, upper, OVL_XATTR_ORIGIN, fh ? fh->buf : NULL,
> >                                  fh ? fh->fb.len : 0, 0);
> >         kfree(fh);
> >
> > --
> > 2.30.2
> 
> After discussing this with Dan Carpenter, this is not a kernel bug,
> it is a smatch bug.

Yeah.  Sorry about that, Su Hui.  The ->buf struct member is not a
pointer, it's an array.  So this isn't really a dereference, it's just
pointer math and foo = fh->buf won't crash even if fh is NULL.

I have written a fix for this in Smatch.  I'll test it a bit before I
push it.

regards,
dan carpenter

Re: [PATCH] ovl: avoid possible NULL dereference

Posted by Su Hui 2 years, 4 months ago

On 2023/9/27 22:39, Dan Carpenter wrote:
> On Wed, Sep 27, 2023 at 05:02:26PM +0300, Amir Goldstein wrote:
>> On Mon, Sep 25, 2023 at 7:52 AM Su Hui <suhui@nfschina.com> wrote:
>>> smatch warn:
>>> fs/overlayfs/copy_up.c:450 ovl_set_origin() warn:
>>> variable dereferenced before check 'fh' (see line 449)
>>>
>>> If 'fh' is NULL, passing NULL instead of 'fh->buf'.
>>>
>>> Signed-off-by: Su Hui <suhui@nfschina.com>
>>> ---
>>>   fs/overlayfs/copy_up.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>>> index d1761ec5866a..086f9176b4d4 100644
>>> --- a/fs/overlayfs/copy_up.c
>>> +++ b/fs/overlayfs/copy_up.c
>>> @@ -446,7 +446,7 @@ int ovl_set_origin(struct ovl_fs *ofs, struct dentry *lower,
>>>          /*
>>>           * Do not fail when upper doesn't support xattrs.
>>>           */
>>> -       err = ovl_check_setxattr(ofs, upper, OVL_XATTR_ORIGIN, fh->buf,
>>> +       err = ovl_check_setxattr(ofs, upper, OVL_XATTR_ORIGIN, fh ? fh->buf : NULL,
>>>                                   fh ? fh->fb.len : 0, 0);
>>>          kfree(fh);
>>>
>>> --
>>> 2.30.2
>> After discussing this with Dan Carpenter, this is not a kernel bug,
>> it is a smatch bug.
> Yeah.  Sorry about that, Su Hui.  The ->buf struct member is not a
> pointer, it's an array.  So this isn't really a dereference, it's just
> pointer math and foo = fh->buf won't crash even if fh is NULL.
Got it, I'm so careless that make this wrong patch.
Really thanks for your reminder!

Su Hui

>
> I have written a fix for this in Smatch.  I'll test it a bit before I
> push it.
>
> regards,
> dan carpenter
>

Re: [PATCH] ovl: avoid possible NULL dereference

Posted by Dan Carpenter 2 years, 4 months ago

On Thu, Sep 28, 2023 at 09:12:01AM +0800, Su Hui wrote:
> Got it, I'm so careless that make this wrong patch.

Not at all.  Your patch didn't break anything and this stuff is subtle.
I've done the same thing myself.

regards,
dan carpenter