diff mbox series

ceph: increment refcount before usage

Message ID 20240201113716.27131-1-ridave@redhat.com
State New
Headers show
Series ceph: increment refcount before usage | expand

Commit Message

ridave@redhat.com Feb. 1, 2024, 11:37 a.m. UTC
From: Rishabh Dave <ridave@redhat.com>

In fs/ceph/caps.c, in "encode_cap_msg()", "use after free" error was
caught by KASAN at this line - 'ceph_buffer_get(arg->xattr_buf);'. This
implies before the refcount could be increment here, it was freed.

In same file, in "handle_cap_grant()" refcount is decremented by this
line - "ceph_buffer_put(ci->i_xattrs.blob);". It appears that a race
occurred and resource was freed by the latter line before the former
line
could increment it.

encode_cap_msg() is called by __send_cap() and __send_cap() is called by
ceph_check_caps() after calling __prep_cap(). __prep_cap() is where
"arg->xattr_buf" is assigned to "ci->i_xattrs.blob" . This is the spot
where the refcount must be increased to prevent "use after free" error.

URL: https://tracker.ceph.com/issues/59259
Signed-off-by: Rishabh Dave <ridave@redhat.com>
---
 fs/ceph/caps.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jeff Layton Feb. 1, 2024, 1:57 p.m. UTC | #1
On Thu, 2024-02-01 at 17:07 +0530, ridave@redhat.com wrote:
> From: Rishabh Dave <ridave@redhat.com>
> 
> In fs/ceph/caps.c, in "encode_cap_msg()", "use after free" error was
> caught by KASAN at this line - 'ceph_buffer_get(arg->xattr_buf);'. This
> implies before the refcount could be increment here, it was freed.
> 
> In same file, in "handle_cap_grant()" refcount is decremented by this
> line - "ceph_buffer_put(ci->i_xattrs.blob);". It appears that a race
> occurred and resource was freed by the latter line before the former
> line
> could increment it.
> 
> encode_cap_msg() is called by __send_cap() and __send_cap() is called by
> ceph_check_caps() after calling __prep_cap(). __prep_cap() is where
> "arg->xattr_buf" is assigned to "ci->i_xattrs.blob" . This is the spot
> where the refcount must be increased to prevent "use after free" error.
> 
> URL: https://tracker.ceph.com/issues/59259
> Signed-off-by: Rishabh Dave <ridave@redhat.com>
> ---
>  fs/ceph/caps.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index 5501c86b337d..0ca7dce48172 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -1452,7 +1452,7 @@ static void __prep_cap(struct cap_msg_args *arg, struct ceph_cap *cap,
>  	if (flushing & CEPH_CAP_XATTR_EXCL) {
>  		arg->old_xattr_buf = __ceph_build_xattrs_blob(ci);
>  		arg->xattr_version = ci->i_xattrs.version;
> -		arg->xattr_buf = ci->i_xattrs.blob;
> +		arg->xattr_buf = ceph_buffer_get(ci->i_xattrs.blob);
>  	} else {
>  		arg->xattr_buf = NULL;
>  		arg->old_xattr_buf = NULL;
> @@ -1553,6 +1553,7 @@ static void __send_cap(struct cap_msg_args *arg, struct ceph_inode_info *ci)
>  	encode_cap_msg(msg, arg);
>  	ceph_con_send(&arg->session->s_con, msg);
>  	ceph_buffer_put(arg->old_xattr_buf);
> +	ceph_buffer_put(arg->xattr_buf);
>  	if (arg->wake)
>  		wake_up_all(&ci->i_cap_wq);
>  }

Nice catch!

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Xiubo Li Feb. 2, 2024, 12:45 a.m. UTC | #2
On 2/1/24 21:57, Jeff Layton wrote:
> On Thu, 2024-02-01 at 17:07 +0530, ridave@redhat.com wrote:
>> From: Rishabh Dave <ridave@redhat.com>
>>
>> In fs/ceph/caps.c, in "encode_cap_msg()", "use after free" error was
>> caught by KASAN at this line - 'ceph_buffer_get(arg->xattr_buf);'. This
>> implies before the refcount could be increment here, it was freed.
>>
>> In same file, in "handle_cap_grant()" refcount is decremented by this
>> line - "ceph_buffer_put(ci->i_xattrs.blob);". It appears that a race
>> occurred and resource was freed by the latter line before the former
>> line
>> could increment it.
>>
>> encode_cap_msg() is called by __send_cap() and __send_cap() is called by
>> ceph_check_caps() after calling __prep_cap(). __prep_cap() is where
>> "arg->xattr_buf" is assigned to "ci->i_xattrs.blob" . This is the spot
>> where the refcount must be increased to prevent "use after free" error.
>>
>> URL: https://tracker.ceph.com/issues/59259
>> Signed-off-by: Rishabh Dave <ridave@redhat.com>
>> ---
>>   fs/ceph/caps.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
>> index 5501c86b337d..0ca7dce48172 100644
>> --- a/fs/ceph/caps.c
>> +++ b/fs/ceph/caps.c
>> @@ -1452,7 +1452,7 @@ static void __prep_cap(struct cap_msg_args *arg, struct ceph_cap *cap,
>>   	if (flushing & CEPH_CAP_XATTR_EXCL) {
>>   		arg->old_xattr_buf = __ceph_build_xattrs_blob(ci);
>>   		arg->xattr_version = ci->i_xattrs.version;
>> -		arg->xattr_buf = ci->i_xattrs.blob;
>> +		arg->xattr_buf = ceph_buffer_get(ci->i_xattrs.blob);
>>   	} else {
>>   		arg->xattr_buf = NULL;
>>   		arg->old_xattr_buf = NULL;
>> @@ -1553,6 +1553,7 @@ static void __send_cap(struct cap_msg_args *arg, struct ceph_inode_info *ci)
>>   	encode_cap_msg(msg, arg);
>>   	ceph_con_send(&arg->session->s_con, msg);
>>   	ceph_buffer_put(arg->old_xattr_buf);
>> +	ceph_buffer_put(arg->xattr_buf);
>>   	if (arg->wake)
>>   		wake_up_all(&ci->i_cap_wq);
>>   }

Looks good to me.

Reviewed-by: Xiubo Li <xiubli@redhat.com>


> Nice catch!
>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>
Thanks Jeff.

- Xiubo
Xiubo Li Feb. 2, 2024, 12:50 a.m. UTC | #3
On 2/1/24 19:37, ridave@redhat.com wrote:
> From: Rishabh Dave <ridave@redhat.com>
>
> In fs/ceph/caps.c, in "encode_cap_msg()", "use after free" error was
> caught by KASAN at this line - 'ceph_buffer_get(arg->xattr_buf);'. This
> implies before the refcount could be increment here, it was freed.
>
> In same file, in "handle_cap_grant()" refcount is decremented by this
> line - "ceph_buffer_put(ci->i_xattrs.blob);". It appears that a race
> occurred and resource was freed by the latter line before the former
> line
> could increment it.
>
> encode_cap_msg() is called by __send_cap() and __send_cap() is called by
> ceph_check_caps() after calling __prep_cap(). __prep_cap() is where
> "arg->xattr_buf" is assigned to "ci->i_xattrs.blob" . This is the spot
> where the refcount must be increased to prevent "use after free" error.
>
> URL: https://tracker.ceph.com/issues/59259
> Signed-off-by: Rishabh Dave <ridave@redhat.com>
> ---
>   fs/ceph/caps.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index 5501c86b337d..0ca7dce48172 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -1452,7 +1452,7 @@ static void __prep_cap(struct cap_msg_args *arg, struct ceph_cap *cap,
>   	if (flushing & CEPH_CAP_XATTR_EXCL) {
>   		arg->old_xattr_buf = __ceph_build_xattrs_blob(ci);
>   		arg->xattr_version = ci->i_xattrs.version;
> -		arg->xattr_buf = ci->i_xattrs.blob;
> +		arg->xattr_buf = ceph_buffer_get(ci->i_xattrs.blob);
>   	} else {
>   		arg->xattr_buf = NULL;
>   		arg->old_xattr_buf = NULL;
> @@ -1553,6 +1553,7 @@ static void __send_cap(struct cap_msg_args *arg, struct ceph_inode_info *ci)
>   	encode_cap_msg(msg, arg);
>   	ceph_con_send(&arg->session->s_con, msg);
>   	ceph_buffer_put(arg->old_xattr_buf);
> +	ceph_buffer_put(arg->xattr_buf);
>   	if (arg->wake)
>   		wake_up_all(&ci->i_cap_wq);
>   }

Applied to the ceph-client's 'testing' branch and will run the tests.

Thanks

- Xiubo
diff mbox series

Patch

diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index 5501c86b337d..0ca7dce48172 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -1452,7 +1452,7 @@  static void __prep_cap(struct cap_msg_args *arg, struct ceph_cap *cap,
 	if (flushing & CEPH_CAP_XATTR_EXCL) {
 		arg->old_xattr_buf = __ceph_build_xattrs_blob(ci);
 		arg->xattr_version = ci->i_xattrs.version;
-		arg->xattr_buf = ci->i_xattrs.blob;
+		arg->xattr_buf = ceph_buffer_get(ci->i_xattrs.blob);
 	} else {
 		arg->xattr_buf = NULL;
 		arg->old_xattr_buf = NULL;
@@ -1553,6 +1553,7 @@  static void __send_cap(struct cap_msg_args *arg, struct ceph_inode_info *ci)
 	encode_cap_msg(msg, arg);
 	ceph_con_send(&arg->session->s_con, msg);
 	ceph_buffer_put(arg->old_xattr_buf);
+	ceph_buffer_put(arg->xattr_buf);
 	if (arg->wake)
 		wake_up_all(&ci->i_cap_wq);
 }