[FIXED] How to disable reading or writing functionality on a proc file?

Issue

I am creating a proc file (/proc/key) that a user can write his decryption_key to it and then this key will be used to decrypt the contents of a buffer stored inside a kernel module. Also, I have another proc entry (/proc/decrypted) that will be used to read the contents of the buffer that stores the decrypted text.

The problem is that I don't want the user to be able to write anything to the (/proc/decrypted) file and I don't want him to read anything from the (/proc/key). How can this be implemented?

I have pointed the corresponding functions inside the file_operations struct to NULL, but obviously, this is going to cause segmentation faults once the user tries them.

How can I prevent reading or writing from a procfs? should I just create functions that have no body and point the file_operations struct to them when needed?

static ssize_t key_proc_write(struct file *filp, const char __user *buf, size_t count, loff_t *ppos)
{
    char temp[128];
    memset(temp, 0, 128);
    int c; 
    c = copy_from_user(temp, buf, count);
 return count;
}


static const struct file_operations Proc_key_fops = {
 .owner = THIS_MODULE,
 .open = hello_proc_open,
 .read = NULL,
 .write = key_proc_write,
 .llseek = seq_lseek,
 .release = single_release,
};  

Solution

If you want to disallow reading, you can just omit explicitly setting the .read field of struct file_operation. If the structure is defined as static and therefore initialized to 0, all fields that are not explicitly overridden will default to NULL, and the kernel will simply not do anything and return an error (I believe -EINVAL) whenever user code tries to call read on your open file.

Alternatively, if you want to return a custom error, you can define a dummy function which only returns an error (like for example return -EFAULT;).

Do you think the way I am "writing" the key into the buffer is the right way to do it ?

This is wrong for multiple reasons.

First, your copy_from_user() is blindly trusting the user count, so this result in a kernel buffer overflow on the temp variable, which is pretty bad. You need to check and/or limit the size first. You are also not checking the return value of copy_from_user(), which you should (and it is not an int, but rather an unsigned long).

static ssize_t key_proc_write(struct file *filp, const char __user *buf, size_t count, loff_t *ppos)
{
    char temp[128];
    memset(temp, 0, 128);

    if (count > 128)
        count = 128; // or alternatively return -EINVAL or another error

    if (copy_from_user(temp, buf, count))
        return -EFAULT;

    return count;
}

Now the code makes more sense, however the variable temp is defined only locally inside the function, and therefore will be lost after the function returns, you will not be able to use it in other file operation functions. If you want to do this, you can use filp->private_data, which is a field of struct file exactly meant for this purpose.

You should create and initialize a buffer on open, and then free it in your release function, something like this:

static int hello_proc_open(struct inode *ino, struct file *filp) 
{
    void *buf = kmalloc(128, GFP_KERNEL);
    if (!buf)
        return -ENOMEM;

    filp->private_data = buf;

    // ... whatever else you need to do

    return 0;
}

static int hello_proc_release(struct inode *ino, struct file *filp) 
{
    kfree(filp->private_data);

    // ... whatever else you need to do

    return 0;
}

Then, in your write you can directly use the buffer after casting it to the correct type:

static ssize_t key_proc_write(struct file *filp, const char __user *buf, size_t count, loff_t *ppos)
{
    char *temp = filp->private_data;
    memset(temp, 0, 128);

    if (count > 128)
        count = 128; // or alternatively return -EINVAL or another error

    if (copy_from_user(temp, buf, count))
        return -EFAULT;

    return count;
}

Finally, I see you are using:

 .llseek = seq_lseek,
 .release = single_release,

Do not do this. You do not need to use pre-defined operations. Do not mix custom functions created by you and other canonical functions like for example those used for sequence files. Sequence files expect some initialization and teardown to be performed, either you use all the seq_ family of file operations, or you do it manually yourself OR you do not use the functions. In your case the last option applies.

You can just set .release to the hello_proc_release() I showed above, and leave .llseek not set (default to NULL).

Answered By - Marco Bonelli

Answer Checked By - Senaida (PHPFixing Volunteer)