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 <[email protected]>
---
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
On Mon, Sep 25, 2023 at 7:52 AM Su Hui <[email protected]> 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 <[email protected]>
> ---
> 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.
On Wed, Sep 27, 2023 at 05:02:26PM +0300, Amir Goldstein wrote:
> On Mon, Sep 25, 2023 at 7:52 AM Su Hui <[email protected]> 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 <[email protected]>
> > ---
> > 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
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 <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
>
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