On Mon, Jul 29 2019, J. Bruce Fields wrote:
> On Fri, Jul 26, 2019 at 06:33:01PM -0400, Dave Wysochanski wrote:
>> The sunrpc cache interface is susceptible to being fooled by a rogue
>> process just reading a 'channel' file. If this happens the kernel
>> may think a valid daemon exists to service the cache when it does not.
>> For example, the following may fool the kernel:
>> cat /proc/net/rpc/auth.unix.gid/channel
>>
>> Change the tracking of readers to writers when considering whether a
>> listener exists as all valid daemon processes either open a channel
>> file O_RDWR or O_WRONLY. While this does not prevent a rogue process
>> from "stealing" a message from the kernel, it does at least improve
>> the kernels perception of whether a valid process servicing the cache
>> exists.
>>
>> Signed-off-by: Dave Wysochanski <dwysocha@xxxxxxxxxx>
>> ---
>> include/linux/sunrpc/cache.h | 6 +++---
>> net/sunrpc/cache.c | 12 ++++++++----
>> 2 files changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/linux/sunrpc/cache.h b/include/linux/sunrpc/cache.h
>> index c7f38e8..f7d086b 100644
>> --- a/include/linux/sunrpc/cache.h
>> +++ b/include/linux/sunrpc/cache.h
>> @@ -107,9 +107,9 @@ struct cache_detail {
>> /* fields for communication over channel */
>> struct list_head queue;
>>
>> - atomic_t readers; /* how many time is /chennel open */
>> - time_t last_close; /* if no readers, when did last close */
>> - time_t last_warn; /* when we last warned about no readers */
>> + atomic_t writers; /* how many time is /channel open */
>> + time_t last_close; /* if no writers, when did last close */
>> + time_t last_warn; /* when we last warned about no writers */
>>
>> union {
>> struct proc_dir_entry *procfs;
>> diff --git a/net/sunrpc/cache.c b/net/sunrpc/cache.c
>> index 6f1528f..a6a6190 100644
>> --- a/net/sunrpc/cache.c
>> +++ b/net/sunrpc/cache.c
>> @@ -373,7 +373,7 @@ void sunrpc_init_cache_detail(struct cache_detail *cd)
>> spin_lock(&cache_list_lock);
>> cd->nextcheck = 0;
>> cd->entries = 0;
>> - atomic_set(&cd->readers, 0);
>> + atomic_set(&cd->writers, 0);
>> cd->last_close = 0;
>> cd->last_warn = -1;
>> list_add(&cd->others, &cache_list);
>> @@ -1029,11 +1029,13 @@ static int cache_open(struct inode *inode, struct file *filp,
>> }
>> rp->offset = 0;
>> rp->q.reader = 1;
>> - atomic_inc(&cd->readers);
>> +
>> spin_lock(&queue_lock);
>> list_add(&rp->q.list, &cd->queue);
>> spin_unlock(&queue_lock);
>> }
>> + if (filp->f_mode & FMODE_WRITE)
>> + atomic_inc(&cd->writers);
>
> This patch would be even simpler if we just modified the condition of
> the preceding if clause:
>
> - if (filp->f_mode & FMODE_READ) {
> + if (filp->f_mode & FMODE_WRITE) {
>
> and then we could drop the following chunk completely.
>
> Is there any reason not to do that?
I can see how this would be tempting, but I think the reason not to do
that is it is ... wrong.
The bulk of the code is for setting up context to support reading, so it
really should be conditional on FMODE_READ.
We always want to set that up, because if a process opens for read, and
not write, we want to respond properly to read requests. This is useful
for debugging.
I think this patch from Dave is good. A process opening for read might
just be inquisitive. A program opening for write is making more of a
commitment to being involved in managing the cache.
Reviewed-by: NeilBrown <neilb@xxxxxxxx>
Thanks,
NeilBrown
>
> Or if the resulting behavior isn't right for write-only openers, we
> could make the condition ((filp->f_mode & FMODE_READ) && (filp->f_mode &
> FMODE_WRITE)).
>
> --b.
>
>> filp->private_data = rp;
>> return 0;
>> }
>> @@ -1062,8 +1064,10 @@ static int cache_release(struct inode *inode, struct file *filp,
>> filp->private_data = NULL;
>> kfree(rp);
>>
>> + }
>> + if (filp->f_mode & FMODE_WRITE) {
>> + atomic_dec(&cd->writers);
>> cd->last_close = seconds_since_boot();
>> - atomic_dec(&cd->readers);
>> }
>> module_put(cd->owner);
>> return 0;
>> @@ -1171,7 +1175,7 @@ static void warn_no_listener(struct cache_detail *detail)
>>
>> static bool cache_listeners_exist(struct cache_detail *detail)
>> {
>> - if (atomic_read(&detail->readers))
>> + if (atomic_read(&detail->writers))
>> return true;
>> if (detail->last_close == 0)
>> /* This cache was never opened */
>> --
>> 1.8.3.1
Attachment:
signature.asc
Description: PGP signature
