Re: [PATCH v3 06/12] cachefiles: add consistency check for copen/cread

[Baokun Li <libaokun@xxxxxxxxxxxxxxx>]

Hi Jingbo,

Thanks for the review!

On 2024/5/23 22:28, Jingbo Xu wrote:
> On 5/22/24 7:43 PM, libaokun@xxxxxxxxxxxxxxx wrote:
> > From: Baokun Li <libaokun1@xxxxxxxxxx>
> >
> > 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 <libaokun1@xxxxxxxxxx>
> > Acked-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> >   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