In btree_split, btree_node_alloc_replacement() is assigned to
n1 and return error code or NULL on failure. n1->c->cache is
passed to block_bytes. So there is a dereference of it
without checks, which may lead to wild pointer dereference or
NULL pointer dereference depending on n1. The initial code only
judge the error code but igore the NULL pointer.
So does n2 and n3.
Fix this bug by adding IS_ERR_OR_NULL check of n1, n2 and n3.
Note that, as a bug found by static analysis, it can be a false
positive or hard to trigger.
Fixes: cafe56359144 ("bcache: A block layer cache")
Signed-off-by: Zheng Wang <[email protected]>
---
drivers/md/bcache/btree.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
index 147c493a989a..d5ed382fc43c 100644
--- a/drivers/md/bcache/btree.c
+++ b/drivers/md/bcache/btree.c
@@ -2206,7 +2206,7 @@ static int btree_split(struct btree *b, struct btree_op *op,
}
n1 = btree_node_alloc_replacement(b, op);
- if (IS_ERR(n1))
+ if (IS_ERR_OR_NULL(n1))
goto err;
split = set_blocks(btree_bset_first(n1),
@@ -2218,12 +2218,12 @@ static int btree_split(struct btree *b, struct btree_op *op,
trace_bcache_btree_node_split(b, btree_bset_first(n1)->keys);
n2 = bch_btree_node_alloc(b->c, op, b->level, b->parent);
- if (IS_ERR(n2))
+ if (IS_ERR_OR_NULL(n2))
goto err_free1;
if (!b->parent) {
n3 = bch_btree_node_alloc(b->c, op, b->level + 1, NULL);
- if (IS_ERR(n3))
+ if (IS_ERR_OR_NULL(n3))
goto err_free2;
}
--
2.25.1
On Thu, Feb 02, 2023 at 07:05:43PM +0800, Zheng Wang wrote:
> In btree_split, btree_node_alloc_replacement() is assigned to
> n1 and return error code or NULL on failure. n1->c->cache is
> passed to block_bytes. So there is a dereference of it
> without checks, which may lead to wild pointer dereference or
> NULL pointer dereference depending on n1. The initial code only
> judge the error code but igore the NULL pointer.
> So does n2 and n3.
>
> Fix this bug by adding IS_ERR_OR_NULL check of n1, n2 and n3.
>
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger.
>
> Fixes: cafe56359144 ("bcache: A block layer cache")
> Signed-off-by: Zheng Wang <[email protected]>
> ---
> drivers/md/bcache/btree.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Note, there is no need to cc: [email protected] on any of these patches that
you send to public mailing lists, as the development and discussion of
them should happen there instead.
thanks,
greg k-h
> 2023年2月2日 19:05，Zheng Wang <[email protected]> 写道：
>
> In btree_split, btree_node_alloc_replacement() is assigned to
> n1 and return error code or NULL on failure. n1->c->cache is
> passed to block_bytes. So there is a dereference of it
> without checks, which may lead to wild pointer dereference or
> NULL pointer dereference depending on n1. The initial code only
> judge the error code but igore the NULL pointer.
> So does n2 and n3.
>
> Fix this bug by adding IS_ERR_OR_NULL check of n1, n2 and n3.
>
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger.
>
> Fixes: cafe56359144 ("bcache: A block layer cache")
> Signed-off-by: Zheng Wang <[email protected]>
Hmm, there should be something to be fixed, but not the non-exist NULL dereference.
If you look inside btree_node_alloc_replacement(), you may find __bch_btree_node_alloc() which does the real work indeed. And yes, I would suggest you to improve a bit inside __bch_btree_node_alloc().
1088 struct btree *__bch_btree_node_alloc(struct cache_set *c, struct btree_op *op,
[snipped]
1093 struct btree *b = ERR_PTR(-EAGAIN);
1094
1095 mutex_lock(&c->bucket_lock);
1096 retry:
1097 if (__bch_bucket_alloc_set(c, RESERVE_BTREE, &k.key, wait))
1098 goto err;
[snipped]
1102
1103 b = mca_alloc(c, op, &k.key, level);
1104 if (IS_ERR(b))
1105 goto err_free;
1106
1107 if (!b) {
1108 cache_bug(c,
1109 "Tried to allocate bucket that was in btree cache");
1110 goto retry;
1111 }
1112
From the above code, at line 1097 if __bch_bucket_alloc_set() returns non-zero value, the code will jump to label err: and returns ERR_PTR(-EAGAIN). But if the code fails at line 1103 and b is set to NULL, at line 1110 the code will jump back to label retry: and calls __bch_bucket_alloc_set() again. If the second time __bch_bucket_alloc_set() returns non-zero value and the code jumps to label err:, a NULL pointer other than ERR_PTR(-EAGAIN) will be returned. This inconsistent return value on same location at line 1097 seems incorrect, where ERR_PTR(-EAGAIN) should be returned IMHO.
Therefore I feel that “b = ERR_PTR(-EAGAIN)” should be moved to the location after label retry:, then btree_node_alloc_replacement() will only return error code, and no NULL pointer returned.
After this change, all following IS_ERR_OR_NULL() checks to btree_node_alloc_replacement() are unnecessary and IS_ERR() just enough, because no NULL will be returned.
This is a nice catch. I’d like to have it to be fixed. I do appreciate if you want to compose two patches for the fix. Otherwise I can handle it myself.
Thanks.
Coly Li
> ---
> drivers/md/bcache/btree.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> index 147c493a989a..d5ed382fc43c 100644
> --- a/drivers/md/bcache/btree.c
> +++ b/drivers/md/bcache/btree.c
> @@ -2206,7 +2206,7 @@ static int btree_split(struct btree *b, struct btree_op *op,
> }
>
> n1 = btree_node_alloc_replacement(b, op);
> - if (IS_ERR(n1))
> + if (IS_ERR_OR_NULL(n1))
> goto err;
>
> split = set_blocks(btree_bset_first(n1),
> @@ -2218,12 +2218,12 @@ static int btree_split(struct btree *b, struct btree_op *op,
> trace_bcache_btree_node_split(b, btree_bset_first(n1)->keys);
>
> n2 = bch_btree_node_alloc(b->c, op, b->level, b->parent);
> - if (IS_ERR(n2))
> + if (IS_ERR_OR_NULL(n2))
> goto err_free1;
>
> if (!b->parent) {
> n3 = bch_btree_node_alloc(b->c, op, b->level + 1, NULL);
> - if (IS_ERR(n3))
> + if (IS_ERR_OR_NULL(n3))
> goto err_free2;
> }
>
> --
> 2.25.1
>
Greg KH <[email protected]> 于2023年2月2日周四 20:08写道：
>
> Note, there is no need to cc: [email protected] on any of these patches that
> you send to public mailing lists, as the development and discussion of
> them should happen there instead.
>
Hi Greg,
Thanks for your kind reminding. I'll remember that in the future.
Best regards,
Zheng Wang
Coly Li <[email protected]> 于2023年2月2日周四 20:22写道：
> Hmm, there should be something to be fixed, but not the non-exist NULL dereference.
>
> If you look inside btree_node_alloc_replacement(), you may find __bch_btree_node_alloc() which does the real work indeed. And yes, I would suggest you to improve a bit inside __bch_btree_node_alloc().
>
> 1088 struct btree *__bch_btree_node_alloc(struct cache_set *c, struct btree_op *op,
> [snipped]
> 1093 struct btree *b = ERR_PTR(-EAGAIN);
> 1094
> 1095 mutex_lock(&c->bucket_lock);
> 1096 retry:
> 1097 if (__bch_bucket_alloc_set(c, RESERVE_BTREE, &k.key, wait))
> 1098 goto err;
> [snipped]
> 1102
> 1103 b = mca_alloc(c, op, &k.key, level);
> 1104 if (IS_ERR(b))
> 1105 goto err_free;
> 1106
> 1107 if (!b) {
> 1108 cache_bug(c,
> 1109 "Tried to allocate bucket that was in btree cache");
> 1110 goto retry;
> 1111 }
> 1112
>
> From the above code, at line 1097 if __bch_bucket_alloc_set() returns non-zero value, the code will jump to label err: and returns ERR_PTR(-EAGAIN). But if the code fails at line 1103 and b is set to NULL, at line 1110 the code will jump back to label retry: and calls __bch_bucket_alloc_set() again. If the second time __bch_bucket_alloc_set() returns non-zero value and the code jumps to label err:, a NULL pointer other than ERR_PTR(-EAGAIN) will be returned. This inconsistent return value on same location at line 1097 seems incorrect, where ERR_PTR(-EAGAIN) should be returned IMHO.
>
> Therefore I feel that “b = ERR_PTR(-EAGAIN)” should be moved to the location after label retry:, then btree_node_alloc_replacement() will only return error code, and no NULL pointer returned.
>
> After this change, all following IS_ERR_OR_NULL() checks to btree_node_alloc_replacement() are unnecessary and IS_ERR() just enough, because no NULL will be returned.
>
> This is a nice catch. I’d like to have it to be fixed. I do appreciate if you want to compose two patches for the fix. Otherwise I can handle it myself.
>
Hi Coly,
Thanks for your reply and detailed explaination! As you explain, I
found __bch_btree_node_alloc may return NULL in some situation. So I
add some more check in upper code.
Your suggestion is more constructive. It'll make the function more
clear for other developer. I'd like to help with the patch. And you
have kindly pointed the right way to fix.
May I merge fix it in one patch with the commit msg "refactor
__bch_btree_node_alloc to avoid poential NULL dereference"? Because I
think if __bch_btree_node_alloc returns
NULL to bch_btree_node_alloc, the function
btree_node_alloc_replacement will strill return NULL to n1 in
btree_split. I think the possibility is low, if it's not proper,
please feel free
to let me know.
Best regards,
Zheng Wang
> 2023年2月2日 22:11，Zheng Hacker <[email protected]> 写道：
>
> Coly Li <[email protected]> 于2023年2月2日周四 20:22写道：
>
>> Hmm, there should be something to be fixed, but not the non-exist NULL dereference.
>>
>> If you look inside btree_node_alloc_replacement(), you may find __bch_btree_node_alloc() which does the real work indeed. And yes, I would suggest you to improve a bit inside __bch_btree_node_alloc().
>>
>> 1088 struct btree *__bch_btree_node_alloc(struct cache_set *c, struct btree_op *op,
>> [snipped]
>> 1093 struct btree *b = ERR_PTR(-EAGAIN);
>> 1094
>> 1095 mutex_lock(&c->bucket_lock);
>> 1096 retry:
>> 1097 if (__bch_bucket_alloc_set(c, RESERVE_BTREE, &k.key, wait))
>> 1098 goto err;
>> [snipped]
>> 1102
>> 1103 b = mca_alloc(c, op, &k.key, level);
>> 1104 if (IS_ERR(b))
>> 1105 goto err_free;
>> 1106
>> 1107 if (!b) {
>> 1108 cache_bug(c,
>> 1109 "Tried to allocate bucket that was in btree cache");
>> 1110 goto retry;
>> 1111 }
>> 1112
>>
>> From the above code, at line 1097 if __bch_bucket_alloc_set() returns non-zero value, the code will jump to label err: and returns ERR_PTR(-EAGAIN). But if the code fails at line 1103 and b is set to NULL, at line 1110 the code will jump back to label retry: and calls __bch_bucket_alloc_set() again. If the second time __bch_bucket_alloc_set() returns non-zero value and the code jumps to label err:, a NULL pointer other than ERR_PTR(-EAGAIN) will be returned. This inconsistent return value on same location at line 1097 seems incorrect, where ERR_PTR(-EAGAIN) should be returned IMHO.
>>
>> Therefore I feel that “b = ERR_PTR(-EAGAIN)” should be moved to the location after label retry:, then btree_node_alloc_replacement() will only return error code, and no NULL pointer returned.
>>
>> After this change, all following IS_ERR_OR_NULL() checks to btree_node_alloc_replacement() are unnecessary and IS_ERR() just enough, because no NULL will be returned.
>>
>> This is a nice catch. I’d like to have it to be fixed. I do appreciate if you want to compose two patches for the fix. Otherwise I can handle it myself.
>>
> Hi Coly,
>
> Thanks for your reply and detailed explaination! As you explain, I
> found __bch_btree_node_alloc may return NULL in some situation. So I
> add some more check in upper code.
> Your suggestion is more constructive. It'll make the function more
> clear for other developer. I'd like to help with the patch. And you
> have kindly pointed the right way to fix.
> May I merge fix it in one patch with the commit msg "refactor
> __bch_btree_node_alloc to avoid poential NULL dereference"? Because I
> think if __bch_btree_node_alloc returns
> NULL to bch_btree_node_alloc, the function
> btree_node_alloc_replacement will strill return NULL to n1 in
> btree_split. I think the possibility is low, if it's not proper,
> please feel free
> to let me know.
This is not a refactor indeed, just a simple fix to __bch_btree_node_alloc() to make the failure behavior of calling __bch_bucket_alloc_set() at line 1097 to be consistent. A.K.A always returning ERR_PTR(-EAGAIN) when it returns failure.
Another optional patch is to change the unnecessary IS_ERR_OR_NULL() to IS_ERR() in proper locations, because after the first fix, NULL won’t be returned indeed. And extra code comments on why IS_ERR() is sufficient might be preferred IMHO.
Thanks.
Coly Li
Coly Li <[email protected]> 于2023年2月2日周四 22:18写道：
>
>
>
> > 2023年2月2日 22:11，Zheng Hacker <[email protected]> 写道：
> >
> > Coly Li <[email protected]> 于2023年2月2日周四 20:22写道：
> >
> >> Hmm, there should be something to be fixed, but not the non-exist NULL dereference.
> >>
> >> If you look inside btree_node_alloc_replacement(), you may find __bch_btree_node_alloc() which does the real work indeed. And yes, I would suggest you to improve a bit inside __bch_btree_node_alloc().
> >>
> >> 1088 struct btree *__bch_btree_node_alloc(struct cache_set *c, struct btree_op *op,
> >> [snipped]
> >> 1093 struct btree *b = ERR_PTR(-EAGAIN);
> >> 1094
> >> 1095 mutex_lock(&c->bucket_lock);
> >> 1096 retry:
> >> 1097 if (__bch_bucket_alloc_set(c, RESERVE_BTREE, &k.key, wait))
> >> 1098 goto err;
> >> [snipped]
> >> 1102
> >> 1103 b = mca_alloc(c, op, &k.key, level);
> >> 1104 if (IS_ERR(b))
> >> 1105 goto err_free;
> >> 1106
> >> 1107 if (!b) {
> >> 1108 cache_bug(c,
> >> 1109 "Tried to allocate bucket that was in btree cache");
> >> 1110 goto retry;
> >> 1111 }
> >> 1112
> >>
> >> From the above code, at line 1097 if __bch_bucket_alloc_set() returns non-zero value, the code will jump to label err: and returns ERR_PTR(-EAGAIN). But if the code fails at line 1103 and b is set to NULL, at line 1110 the code will jump back to label retry: and calls __bch_bucket_alloc_set() again. If the second time __bch_bucket_alloc_set() returns non-zero value and the code jumps to label err:, a NULL pointer other than ERR_PTR(-EAGAIN) will be returned. This inconsistent return value on same location at line 1097 seems incorrect, where ERR_PTR(-EAGAIN) should be returned IMHO.
> >>
> >> Therefore I feel that “b = ERR_PTR(-EAGAIN)” should be moved to the location after label retry:, then btree_node_alloc_replacement() will only return error code, and no NULL pointer returned.
> >>
> >> After this change, all following IS_ERR_OR_NULL() checks to btree_node_alloc_replacement() are unnecessary and IS_ERR() just enough, because no NULL will be returned.
> >>
> >> This is a nice catch. I’d like to have it to be fixed. I do appreciate if you want to compose two patches for the fix. Otherwise I can handle it myself.
> >>
> > Hi Coly,
> >
> > Thanks for your reply and detailed explaination! As you explain, I
> > found __bch_btree_node_alloc may return NULL in some situation. So I
> > add some more check in upper code.
> > Your suggestion is more constructive. It'll make the function more
> > clear for other developer. I'd like to help with the patch. And you
> > have kindly pointed the right way to fix.
> > May I merge fix it in one patch with the commit msg "refactor
> > __bch_btree_node_alloc to avoid poential NULL dereference"? Because I
> > think if __bch_btree_node_alloc returns
> > NULL to bch_btree_node_alloc, the function
> > btree_node_alloc_replacement will strill return NULL to n1 in
> > btree_split. I think the possibility is low, if it's not proper,
> > please feel free
> > to let me know.
>
> This is not a refactor indeed, just a simple fix to __bch_btree_node_alloc() to make the failure behavior of calling __bch_bucket_alloc_set() at line 1097 to be consistent. A.K.A always returning ERR_PTR(-EAGAIN) when it returns failure.
>
> Another optional patch is to change the unnecessary IS_ERR_OR_NULL() to IS_ERR() in proper locations, because after the first fix, NULL won’t be returned indeed. And extra code comments on why IS_ERR() is sufficient might be preferred IMHO.
>
Got it! Will do right now. Thanks agagin for your clear description
about the fix.
Thanks,
Zheng Wang