From: Baokun Li <[email protected]>
This prevents malicious processes from completing random copen/cread
requests and crashing the system. Added checks are listed below:
* Generic, copen can only complete open requests, and cread can only
complete read requests.
* For copen, ondemand_id must not be 0, because this indicates that the
request has not been read by the daemon.
* For cread, the object corresponding to fd and req should be the same.
Signed-off-by: Baokun Li <[email protected]>
Acked-by: Jeff Layton <[email protected]>
---
fs/cachefiles/ondemand.c | 27 ++++++++++++++++++++-------
1 file changed, 20 insertions(+), 7 deletions(-)
diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
index bb94ef6a6f61..898fab68332b 100644
--- a/fs/cachefiles/ondemand.c
+++ b/fs/cachefiles/ondemand.c
@@ -82,12 +82,12 @@ static loff_t cachefiles_ondemand_fd_llseek(struct file *filp, loff_t pos,
}
static long cachefiles_ondemand_fd_ioctl(struct file *filp, unsigned int ioctl,
- unsigned long arg)
+ unsigned long id)
{
struct cachefiles_object *object = filp->private_data;
struct cachefiles_cache *cache = object->volume->cache;
struct cachefiles_req *req;
- unsigned long id;
+ XA_STATE(xas, &cache->reqs, id);
if (ioctl != CACHEFILES_IOC_READ_COMPLETE)
return -EINVAL;
@@ -95,10 +95,15 @@ static long cachefiles_ondemand_fd_ioctl(struct file *filp, unsigned int ioctl,
if (!test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags))
return -EOPNOTSUPP;
- id = arg;
- req = xa_erase(&cache->reqs, id);
- if (!req)
+ xa_lock(&cache->reqs);
+ req = xas_load(&xas);
+ if (!req || req->msg.opcode != CACHEFILES_OP_READ ||
+ req->object != object) {
+ xa_unlock(&cache->reqs);
return -EINVAL;
+ }
+ xas_store(&xas, NULL);
+ xa_unlock(&cache->reqs);
trace_cachefiles_ondemand_cread(object, id);
complete(&req->done);
@@ -126,6 +131,7 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
unsigned long id;
long size;
int ret;
+ XA_STATE(xas, &cache->reqs, 0);
if (!test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags))
return -EOPNOTSUPP;
@@ -149,9 +155,16 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
if (ret)
return ret;
- req = xa_erase(&cache->reqs, id);
- if (!req)
+ xa_lock(&cache->reqs);
+ xas.xa_index = id;
+ req = xas_load(&xas);
+ if (!req || req->msg.opcode != CACHEFILES_OP_OPEN ||
+ !req->object->ondemand->ondemand_id) {
+ xa_unlock(&cache->reqs);
return -EINVAL;
+ }
+ xas_store(&xas, NULL);
+ xa_unlock(&cache->reqs);
/* fail OPEN request if copen format is invalid */
ret = kstrtol(psize, 0, &size);
--
2.39.2
On 5/22/24 7:43 PM, [email protected] wrote:
> From: Baokun Li <[email protected]>
>
> This prevents malicious processes from completing random copen/cread
> requests and crashing the system. Added checks are listed below:
>
> * Generic, copen can only complete open requests, and cread can only
> complete read requests.
> * For copen, ondemand_id must not be 0, because this indicates that the
> request has not been read by the daemon.
> * For cread, the object corresponding to fd and req should be the same.
>
> Signed-off-by: Baokun Li <[email protected]>
> Acked-by: Jeff Layton <[email protected]>
> ---
> fs/cachefiles/ondemand.c | 27 ++++++++++++++++++++-------
> 1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
> index bb94ef6a6f61..898fab68332b 100644
> --- a/fs/cachefiles/ondemand.c
> +++ b/fs/cachefiles/ondemand.c
> @@ -82,12 +82,12 @@ static loff_t cachefiles_ondemand_fd_llseek(struct file *filp, loff_t pos,
> }
>
> static long cachefiles_ondemand_fd_ioctl(struct file *filp, unsigned int ioctl,
> - unsigned long arg)
> + unsigned long id)
> {
> struct cachefiles_object *object = filp->private_data;
> struct cachefiles_cache *cache = object->volume->cache;
> struct cachefiles_req *req;
> - unsigned long id;
> + XA_STATE(xas, &cache->reqs, id);
>
> if (ioctl != CACHEFILES_IOC_READ_COMPLETE)
> return -EINVAL;
> @@ -95,10 +95,15 @@ static long cachefiles_ondemand_fd_ioctl(struct file *filp, unsigned int ioctl,
> if (!test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags))
> return -EOPNOTSUPP;
>
> - id = arg;
> - req = xa_erase(&cache->reqs, id);
> - if (!req)
> + xa_lock(&cache->reqs);
> + req = xas_load(&xas);
> + if (!req || req->msg.opcode != CACHEFILES_OP_READ ||
> + req->object != object) {
> + xa_unlock(&cache->reqs);
> return -EINVAL;
> + }
> + xas_store(&xas, NULL);
> + xa_unlock(&cache->reqs);
>
> trace_cachefiles_ondemand_cread(object, id);
> complete(&req->done);
> @@ -126,6 +131,7 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
> unsigned long id;
> long size;
> int ret;
> + XA_STATE(xas, &cache->reqs, 0);
>
> if (!test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags))
> return -EOPNOTSUPP;
> @@ -149,9 +155,16 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
> if (ret)
> return ret;
>
> - req = xa_erase(&cache->reqs, id);
> - if (!req)
> + xa_lock(&cache->reqs);
> + xas.xa_index = id;
> + req = xas_load(&xas);
> + if (!req || req->msg.opcode != CACHEFILES_OP_OPEN ||
> + !req->object->ondemand->ondemand_id) {
For a valid opened object, I think ondemand_id shall > 0. When the
copen is for the object which is in the reopening state, ondemand_id can
be CACHEFILES_ONDEMAND_ID_CLOSED (actually -1)?
Otherwise looks good to me.
--
Thanks,
Jingbo
Hi Jingbo,
Thanks for the review!
On 2024/5/23 22:28, Jingbo Xu wrote:
>
> On 5/22/24 7:43 PM, [email protected] wrote:
>> From: Baokun Li <[email protected]>
>>
>> This prevents malicious processes from completing random copen/cread
>> requests and crashing the system. Added checks are listed below:
>>
>> * Generic, copen can only complete open requests, and cread can only
>> complete read requests.
>> * For copen, ondemand_id must not be 0, because this indicates that the
>> request has not been read by the daemon.
>> * For cread, the object corresponding to fd and req should be the same.
>>
>> Signed-off-by: Baokun Li <[email protected]>
>> Acked-by: Jeff Layton <[email protected]>
>> ---
>> fs/cachefiles/ondemand.c | 27 ++++++++++++++++++++-------
>> 1 file changed, 20 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
>> index bb94ef6a6f61..898fab68332b 100644
>> --- a/fs/cachefiles/ondemand.c
>> +++ b/fs/cachefiles/ondemand.c
>> @@ -82,12 +82,12 @@ static loff_t cachefiles_ondemand_fd_llseek(struct file *filp, loff_t pos,
>> }
>>
>> static long cachefiles_ondemand_fd_ioctl(struct file *filp, unsigned int ioctl,
>> - unsigned long arg)
>> + unsigned long id)
>> {
>> struct cachefiles_object *object = filp->private_data;
>> struct cachefiles_cache *cache = object->volume->cache;
>> struct cachefiles_req *req;
>> - unsigned long id;
>> + XA_STATE(xas, &cache->reqs, id);
>>
>> if (ioctl != CACHEFILES_IOC_READ_COMPLETE)
>> return -EINVAL;
>> @@ -95,10 +95,15 @@ static long cachefiles_ondemand_fd_ioctl(struct file *filp, unsigned int ioctl,
>> if (!test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags))
>> return -EOPNOTSUPP;
>>
>> - id = arg;
>> - req = xa_erase(&cache->reqs, id);
>> - if (!req)
>> + xa_lock(&cache->reqs);
>> + req = xas_load(&xas);
>> + if (!req || req->msg.opcode != CACHEFILES_OP_READ ||
>> + req->object != object) {
>> + xa_unlock(&cache->reqs);
>> return -EINVAL;
>> + }
>> + xas_store(&xas, NULL);
>> + xa_unlock(&cache->reqs);
>>
>> trace_cachefiles_ondemand_cread(object, id);
>> complete(&req->done);
>> @@ -126,6 +131,7 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
>> unsigned long id;
>> long size;
>> int ret;
>> + XA_STATE(xas, &cache->reqs, 0);
>>
>> if (!test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags))
>> return -EOPNOTSUPP;
>> @@ -149,9 +155,16 @@ int cachefiles_ondemand_copen(struct cachefiles_cache *cache, char *args)
>> if (ret)
>> return ret;
>>
>> - req = xa_erase(&cache->reqs, id);
>> - if (!req)
>> + xa_lock(&cache->reqs);
>> + xas.xa_index = id;
>> + req = xas_load(&xas);
>> + if (!req || req->msg.opcode != CACHEFILES_OP_OPEN ||
>> + !req->object->ondemand->ondemand_id) {
> For a valid opened object, I think ondemand_id shall > 0. When the
> copen is for the object which is in the reopening state, ondemand_id can
> be CACHEFILES_ONDEMAND_ID_CLOSED (actually -1)?
If ondemand_id is -1, there are two scenarios:
* This could be a restore/reopen request that has not yet get_fd;
* The request is being processed by the daemon but its anonymous
fd has been closed.
In the first case, there is no argument for not allowing copen.
In the latter case, however, the closing of an anonymous fd may
not be malicious, so if a copen delete request fails, the OPEN
request will not be processed until RESTORE lets it be processed
by the daemon again. However, RESTORE is not a frequent operation,
so if only one anonymous fd is accidentally closed, this may result
in a hung.
So in later patches, we ensure that fd is valid (i.e. ondemand_id > 0)
when setting the object to OPEN state and do not prevent it
from removing the request here.
If ondemand_id is 0, then it can be confirmed that the req has not
been initialised, so the copen must be malicious at this point, so it
is not allowed to complete the request. This is an instantaneous
state, and the request can be processed normally after the daemon
has read it properly. So there won't be any side effects here.
--
With Best Regards,
Baokun Li
On 5/24/24 10:28 AM, Baokun Li wrote:
> Hi Jingbo,
>
> Thanks for the review!
>
> On 2024/5/23 22:28, Jingbo Xu wrote:
>>
>> On 5/22/24 7:43 PM, [email protected] wrote:
>>> From: Baokun Li <[email protected]>
>>>
>>> This prevents malicious processes from completing random copen/cread
>>> requests and crashing the system. Added checks are listed below:
>>>
>>> * Generic, copen can only complete open requests, and cread can only
>>> complete read requests.
>>> * For copen, ondemand_id must not be 0, because this indicates
>>> that the
>>> request has not been read by the daemon.
>>> * For cread, the object corresponding to fd and req should be the
>>> same.
>>>
>>> Signed-off-by: Baokun Li <[email protected]>
>>> Acked-by: Jeff Layton <[email protected]>
>>> ---
>>> fs/cachefiles/ondemand.c | 27 ++++++++++++++++++++-------
>>> 1 file changed, 20 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/cachefiles/ondemand.c b/fs/cachefiles/ondemand.c
>>> index bb94ef6a6f61..898fab68332b 100644
>>> --- a/fs/cachefiles/ondemand.c
>>> +++ b/fs/cachefiles/ondemand.c
>>> @@ -82,12 +82,12 @@ static loff_t
>>> cachefiles_ondemand_fd_llseek(struct file *filp, loff_t pos,
>>> }
>>> static long cachefiles_ondemand_fd_ioctl(struct file *filp,
>>> unsigned int ioctl,
>>> - unsigned long arg)
>>> + unsigned long id)
>>> {
>>> struct cachefiles_object *object = filp->private_data;
>>> struct cachefiles_cache *cache = object->volume->cache;
>>> struct cachefiles_req *req;
>>> - unsigned long id;
>>> + XA_STATE(xas, &cache->reqs, id);
>>> if (ioctl != CACHEFILES_IOC_READ_COMPLETE)
>>> return -EINVAL;
>>> @@ -95,10 +95,15 @@ static long cachefiles_ondemand_fd_ioctl(struct
>>> file *filp, unsigned int ioctl,
>>> if (!test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags))
>>> return -EOPNOTSUPP;
>>> - id = arg;
>>> - req = xa_erase(&cache->reqs, id);
>>> - if (!req)
>>> + xa_lock(&cache->reqs);
>>> + req = xas_load(&xas);
>>> + if (!req || req->msg.opcode != CACHEFILES_OP_READ ||
>>> + req->object != object) {
>>> + xa_unlock(&cache->reqs);
>>> return -EINVAL;
>>> + }
>>> + xas_store(&xas, NULL);
>>> + xa_unlock(&cache->reqs);
>>> trace_cachefiles_ondemand_cread(object, id);
>>> complete(&req->done);
>>> @@ -126,6 +131,7 @@ int cachefiles_ondemand_copen(struct
>>> cachefiles_cache *cache, char *args)
>>> unsigned long id;
>>> long size;
>>> int ret;
>>> + XA_STATE(xas, &cache->reqs, 0);
>>> if (!test_bit(CACHEFILES_ONDEMAND_MODE, &cache->flags))
>>> return -EOPNOTSUPP;
>>> @@ -149,9 +155,16 @@ int cachefiles_ondemand_copen(struct
>>> cachefiles_cache *cache, char *args)
>>> if (ret)
>>> return ret;
>>> - req = xa_erase(&cache->reqs, id);
>>> - if (!req)
>>> + xa_lock(&cache->reqs);
>>> + xas.xa_index = id;
>>> + req = xas_load(&xas);
>>> + if (!req || req->msg.opcode != CACHEFILES_OP_OPEN ||
>>> + !req->object->ondemand->ondemand_id) {
>> For a valid opened object, I think ondemand_id shall > 0. When the
>> copen is for the object which is in the reopening state, ondemand_id can
>> be CACHEFILES_ONDEMAND_ID_CLOSED (actually -1)?
> If ondemand_id is -1, there are two scenarios:
> * This could be a restore/reopen request that has not yet get_fd;
> * The request is being processed by the daemon but its anonymous
> fd has been closed.
>
> In the first case, there is no argument for not allowing copen.
> In the latter case, however, the closing of an anonymous fd may
> not be malicious, so if a copen delete request fails, the OPEN
> request will not be processed until RESTORE lets it be processed
> by the daemon again. However, RESTORE is not a frequent operation,
> so if only one anonymous fd is accidentally closed, this may result
> in a hung.
>
> So in later patches, we ensure that fd is valid (i.e. ondemand_id > 0)
> when setting the object to OPEN state and do not prevent it
> from removing the request here.
>
> If ondemand_id is 0, then it can be confirmed that the req has not
> been initialised, so the copen must be malicious at this point, so it
> is not allowed to complete the request. This is an instantaneous
> state, and the request can be processed normally after the daemon
> has read it properly. So there won't be any side effects here.
>
case 1 is literally illegal, while case 2 is permissible but has no way
to be distinguished from case 1. As the patch itself is only
best-effort, so it LGTM.
Reviewed-by: Jingbo Xu <[email protected]>
--
Thanks,
Jingbo