dht: fix use after free issue in dht_setxattr_mds_cbk

[mohit84]

It is a day one bug when a feature was implemented, the
issue was not caught earlier because the client crash
depends on the environment.The client was crashing because
during wind it was not passing cookie and in cbk it
was trying to access cookie.

Solution: Pass a xlator_t as a cookie during STACK_WIND_COOKIE
to avoid a crash.

Fixes: #3732
Change-Id: I5fd75e90a94d852093dfa40fb37d4211b4e9fdfb
Signed-off-by: Mohit Agrawal moagrawa@redhat.com

[mohit84]

/run regression

[mohit84]

/run regression

[gluster-ant]

1 test(s) failed
./tests/basic/distribute/spare_file_rebalance.t

0 test(s) generated core

8 test(s) needed retry
./tests/000-flaky/basic_distribute_rebal-all-nodes-migrate.t
./tests/000-flaky/basic_mount-nfs-auth.t
./tests/000-flaky/bugs_distribute_bug-1117851.t
./tests/000-flaky/bugs_glusterd_bug-857330/normal.t
./tests/000-flaky/bugs_glusterd_bug-857330/xml.t
./tests/000-flaky/bugs_nfs_bug-1116503.t
./tests/000-flaky/glusterd-restart-shd-mux.t
./tests/basic/distribute/spare_file_rebalance.t

7 flaky test(s) marked as success even though they failed
./tests/000-flaky/basic_distribute_rebal-all-nodes-migrate.t
./tests/000-flaky/basic_mount-nfs-auth.t
./tests/000-flaky/bugs_distribute_bug-1117851.t
./tests/000-flaky/bugs_glusterd_bug-857330/normal.t
./tests/000-flaky/bugs_glusterd_bug-857330/xml.t
./tests/000-flaky/bugs_nfs_bug-1116503.t
./tests/000-flaky/glusterd-restart-shd-mux.t
https://build.gluster.org/job/gh_centos7-regression/2744/

[mohit84]

/run regression

[xhernandez]

Isn't this frame the same that the callback receives as its first argument ? why do we need to pass it as a cookie ? we could just change the callback to use the frame instead of the cookie.

However it seems to me that the callbacks are using the cookie to get the name of the subvolume where the request was sent. In this case, not passing the cookie is the right thing to do because when STACK_WIND() is called, it assigns the new frame created for the subvolume to the cookie, so the callback receives the frame of the subvolume, not its own.

I'm not sure what was the problem you saw, but maybe it was caused because the frame was destroyed by the subvolume itself. I don't know.

Anyway, I don't like this implicit way to passing frames that are already completed and could potentially be destroyed. Why not just pass local->mds_subvol as the cookie ? it should be enough for the purposes of the callback.

[mohit84]

The problem was callback function was trying to access the volume name from the cookie but we are calling callback function through STACK_WIND not STACK_WIND_COOKIE. Yes there are both ways to solve the issue either we need to use STACK_WIND_COOKIE or change the callback function, the callback function is using a cookie to print the mds subvol name the same we can access through frame->local also.

[xhernandez]

STACK_WIND also initializes the cookie to point to the frame created for the subvolume, so the code wasn't entirely wrong:

    295 #define STACK_WIND_COMMON(frame, rfn, has_cookie, cky, obj, fn, params...)     \
    296     do {                                                                       \
    297         call_frame_t *_new = NULL;                                             \
    298         xlator_t *old_THIS = NULL;                                             \
    299         typeof(fn) next_xl_fn = fn;                                            \
    300                                                                                \
    301         _new = mem_get0(frame->root->pool->frame_mem_pool);                    \
    302         if (caa_unlikely(!_new)) {                                             \
    303             break;                                                             \
    304         }                                                                      \
    305         typeof(fn##_cbk) tmp_cbk = rfn;                                        \
    306         _new->root = frame->root;                                              \
    307         _new->parent = frame;                                                  \
    308         LOCK_INIT(&_new->lock);                                                \
    309         /* (void *) is required for avoiding gcc warning */                    \
    310         _new->cookie = ((has_cookie == 1) ? (void *)(cky) : (void *)_new);     \

So what the code was doing was correct, cookie had the correct value. I'm not sure what codepath leads to a use after free given that we always call callbacks synchronously from the child xlator. I'll check it.

However, in your code you pass frame as the cookie. frame is the current frame, not the frame created for the subvolume, so when coookie->this->name is used, the name will be the dht's name, not the name of the subvolume, which was the intention.

[xhernandez]

After looking at the stack traces from the address sanitizer, I would say that the problem is that a removexattr request has been unwound, which will eventually destroy the entire stack, while a request sent to a subvolume was still pending.

However I haven't identified where this can happen yet.

[mohit84]

After looking at the stack traces from the address sanitizer, I would say that the problem is that a removexattr request has been unwound, which will eventually destroy the entire stack, while a request sent to a subvolume was still pending.

However I haven't identified where this can happen yet.

Thanks for identifying it , I will check it again and update.

[SoumyaWind]

Hi @mohit84 ,

I am working on CVE-2022-48340. So I just want to check if there any update on this? Or any other commit that can fix this issue.

[rohitkuma1313]

Any update on this? @mohit84

[gluster-ant]

!!! Couldn't read commit file !!!