0

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,
};
Marco Bonelli
  • 63,369
  • 21
  • 118
  • 128
Mina Ashraf
  • 353
  • 1
  • 12
  • Point them to a function that returns an error code such as `EPERM` – Barmar Apr 16 '21 at 21:26
  • Also the `open()` function can check whether they specify `O_RDONLY`, `O_WRONLY`, or `O_RDWR` and return an error for invalid direction. – Barmar Apr 16 '21 at 21:27
  • @Barmar how can I point them to these functions? can you give an example? also how can I set the permissions to read-only as you suggested? this will do exactly as I want – Mina Ashraf Apr 16 '21 at 21:30
  • What do you mean? Just write a function `key_proc_read` that returns an error, and use `.read = key_proc_read` – Barmar Apr 16 '21 at 21:31
  • Write a function `key_proc_open` that checks the open mode. – Barmar Apr 16 '21 at 21:32
  • @Barmar Oh I got it now. I thought there were some predefined functions that I could use. – Mina Ashraf Apr 16 '21 at 21:32
  • @Barmar Also another small question. Do you think the way I am "writing" the key into the buffer is the right way to do it ? – Mina Ashraf Apr 16 '21 at 21:33
  • You're copying into a local variable that goes away as soon as the function returns. You need to save the key somewhere permanent. I don't know much about device drivers, but I think there should be a data block associated with each open file, that's where you need to save it. – Barmar Apr 16 '21 at 21:38

1 Answers1

2

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).

Marco Bonelli
  • 63,369
  • 21
  • 118
  • 128