Some programming 101 feedback below:
On Thu, Sep 23, 2010 at 12:47 PM, Bond <jamesbond.2k.g@xxxxxxxxx> wrote:
> I wrote a small hello world type of character device driver.
> When I type echo -n "abcdef" > /dev/bond
> and do a cat /dev/bond
> then only last "f" of above input abcdef is displayed rest nothing is
> displayed.
> I asked this question earlier and some people suggested me some
> modifications
> I have done and experimented all that but I am unable to catch the error.
> Can some one point out the error?
> Here is the code
>
> /* Necessary includes for device drivers */
> #include <linux/init.h>
> //#include <linux/config.h>
> #include <linux/module.h>
> #include <linux/kernel.h> /* printk() */
> #include <linux/slab.h> /* kmalloc() */
> #include <linux/fs.h> /* everything... */
> #include <linux/errno.h> /* error codes */
> #include <linux/types.h> /* size_t */
> #include <linux/proc_fs.h>
> #include <linux/fcntl.h> /* O_ACCMODE */
> #include <asm/system.h> /* cli(), *_flags */
> #include <asm/uaccess.h> /* copy_from/to_user */
> MODULE_LICENSE("Dual BSD/GPL");
> /* Declaration of memory.c functions */
> int memory_open(struct inode *inode, struct file *filp);
> int memory_release(struct inode *inode, struct file *filp);
> ssize_t memory_read(struct file *filp, char *buf, size_t count, loff_t
> *f_pos);
> ssize_t memory_write(struct file *filp, char *buf, size_t count, loff_t
> *f_pos);
> void memory_exit(void);
> int memory_init(void);
> /* Structure that declares the usual file */
> /* access functions */
> struct file_operations memory_fops = {
> read: memory_read,
> write: memory_write,
> open: memory_open,
> release: memory_release
> };
> /* Declaration of the init and exit functions */
> module_init(memory_init);
> module_exit(memory_exit);
> /* Global variables of the driver */
> /* Major number */
> int memory_major = 60;
> /* Buffer to store data */
> char *memory_buffer;
> int memory_init(void) {
> int result;
> /* Registering device */
> result = register_chrdev(memory_major, "bond", &memory_fops);
> if (result < 0) {
> printk(KERN_ALERT "memory: cannot obtain major number %d\n",
> memory_major);
> return result;
> }
> /* Allocating memory for the buffer */
> memory_buffer = kmalloc(1, GFP_KERNEL);
1 is the number of bytes your allocating, it needs to be your max
buffer length. So are by design writing a buffer overflow the way you
have it. That is one of the typical security failures that can allow
a major breach.
Change it to:
#define MY_BUFFER_SIZE
memory_buffer = kmalloc(MY_BUFFER_SIZE, GFP_KERNEL);
> if (!memory_buffer) {
> result = -ENOMEM;
> goto fail;
> }
> memset(memory_buffer, 0, 10);
Never hard code a length. At least use a define. This becomes
memset(memory_buffer, 0, MY_BUFFER_SIZE);
> printk(KERN_ALERT "Inserting bond module\n");
> return 0;
> fail:
> memory_exit();
> return result;
> }
>
> void memory_exit(void) {
> /* Freeing the major number */
> unregister_chrdev(memory_major, "bond");
> /* Freeing buffer memory */
> if (memory_buffer) {
> kfree(memory_buffer);
> }
> printk( KERN_ALERT "Removing bond module\n");
> }
>
> int memory_open(struct inode *inode, struct file *filp) {
> /* Success */
> return 0;
> }
>
> int memory_release(struct inode *inode, struct file *filp) {
>
> /* Success */
> return 0;
> }
>
> ssize_t memory_read(struct file *filp, char *buf,
> size_t count, loff_t *f_pos) {
>
> /* Transfering data to user space */
> copy_to_user(buf,memory_buffer,10);
again, don't have 10 hardcoded, but the read should actually work as
is except if the userspace buffer is too small, then your going to
overflow it with untold damage.
You need:
copy_to_user(buf,memory_buffer,min(count, MY_BUFFER_SIZE));
> /* Changing reading position as best suits */
> if (*f_pos == 0) {
> *f_pos+=1;
> return 1;
> } else {
> return 0;
> }
Since you don't use f_pos, I have no idea why you're don this. Future
plans I assume. While troubleshooting I'm surprised you didn't
comment this out.
I would often just wrap it in a #ifdef until you get the basics working.
> }
> ssize_t memory_write( struct file *filp, char *buf,
> size_t count, loff_t *f_pos) {
> char *tmp;
> tmp=buf+count-1;
tmp now points at the last char in buf. Clearly not what you want.
I can't even think of a logical explanation for what you're trying to
do. I'd just delete tmp and its assignment. Those 2 lines offer no
value I see.
> copy_from_user(memory_buffer,tmp,10);
Try this instead
copy_from_user(memory_buffer, buf, min(count, MY_BUFFER_SIZE));
> return 1;
> }
It looks like you're just randomly cutting and pasting code from
various places and have no idea what it really does.
I'm putting you in my autodelete list, so I won't be responding to you again.
Greg
--
To unsubscribe from this list: send an email with
"unsubscribe kernelnewbies" to ecartis@xxxxxxxxxxxx
Please read the FAQ at http://kernelnewbies.org/FAQ
