avoid NULL deference in ext2_xattr_get

Prev: parser: define __builtin_unreachable
Next: [PATCH] KVM: PIT: stop vpit before irq_routing freed

From: Dan Carpenter on 10 Jul 2010 05:20

---

Good catch. Your change is correct but can you resend it?

On Sat, Jul 10, 2010 at 04:07:28PM +0800, shenghui wrote:
> >From adc1fa6535034db3b6d8deebda6ec7eaa8bfd2f8 Mon Sep 17 00:00:00 2001
> From: Wang Sheng-Hui <crosslonelyover(a)gmail.com>
> Date: Sat, 10 Jul 2010 16:05:53 +0800
> Subject: [PATCH] avoid NULL deference in ext2_xattr_get
>

The longer description of the bug should go here.

>
> Signed-off-by: Wang Sheng-Hui <crosslonelyover(a)gmail.com>
> ---
> fs/ext2/xattr.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index 7c39157..81ec1c6 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -156,11 +156,12 @@ ext2_xattr_get(struct inode *inode, int
> name_index, const char *name,
^^^^^^

Your email client line wrapped the patch. Please send it to yourself
and then save the email including headers and everything.
cd linux-2.6
cat ~/your-email-text | patch -p1

If it applies cleanly then resend it to the list.

regards,
dan carpenter


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: Dan Carpenter on 10 Jul 2010 05:30

---

Another way to fix this would be to say:

ea_idebug(inode, "name=%d.%s, buffer=%p, buffer_size=%ld",
- name_index, name, buffer, (long)buffer_size);
+ name_index, name ?: "(null)", buffer, (long)buffer_size);

That would help for debugging.

regards,
dan carpenter


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: David Beal on 10 Jul 2010 15:20

---

Hi Sheng-Hui,

There is no NULL dereference in this particular case. In Linux's printing
routines, printk("%s",(char*)0) is safe, and would result in a "<NULL>" token
being printed.

Indicating that the "name" argument is "<NULL>" in the output of the ea_debug
routine is clearly valuable, as its purpose is to emit debugging messages.

If your patch were applied, then a log message indicating that "name" is
"<NULL>" would not appear. Adding ternary operator inside printk() argument
body is also not necessary.

Specifically, in lib/vsnprintf.c, which services printk, a check against NULL
is made for the string format specifier, %s:

lib/vsnprintf.c:
....
static char *string(char *buf, char *end, char *s, struct printf_spec spec)
{
int len, i;

if ((unsigned long)s < PAGE_SIZE)
s = "<NULL>";
....

The NULL check inside snprintf is GOOD, as it makes printk safer, and removes
the need for the thousands of conditional branches that would need to be done
OUTSIDE the printk function in order to check whether the argument to %s is
NULL.

Branch prediction has an easier time working with a branch in a function that
is more often in the cache.

Thank you,
David Beal


On Sat, Jul 10, 2010 at 04:07:28PM +0800, shenghui wrote:
> Hi,
>
> I walked through ext2 code, and found one potential NULL deference
> in ext2/xattr.c. The version is 2.6.35-rc4, while earlier versions have the
> same problem.
> If you configure EXT2_XATTR_DEBUG, you'll get:
> # define ea_idebug(inode, f...) do { \
> printk(KERN_DEBUG "inode %s:%ld: ", \
> inode->i_sb->s_id, inode->i_ino); \
> printk(f); \
> printk("\n"); \
> } while (0)
>
> In ext2/xttr.c ext2_xattr_get, NULL pointer check is done after
> ea_idebug call, so some may hit NULL deference here.
> ext2_xattr_get(struct inode *inode, int name_index, const char *name,
> void *buffer, size_t buffer_size)
> {
> struct buffer_head *bh = NULL;
> struct ext2_xattr_entry *entry;
> size_t name_len, size;
> char *end;
> int error;
>
> ea_idebug(inode, "name=%d.%s, buffer=%p, buffer_size=%ld",
> name_index, name, buffer, (long)buffer_size);
>
> if (name == NULL)
> return -EINVAL;
>
>
> Following is my patch. Please check it.
> The patch is against kernel 2.6.35-rc4.
>
>
> From adc1fa6535034db3b6d8deebda6ec7eaa8bfd2f8 Mon Sep 17 00:00:00 2001
> From: Wang Sheng-Hui <crosslonelyover(a)gmail.com>
> Date: Sat, 10 Jul 2010 16:05:53 +0800
> Subject: [PATCH] avoid NULL deference in ext2_xattr_get
>
>
> Signed-off-by: Wang Sheng-Hui <crosslonelyover(a)gmail.com>
> ---
> fs/ext2/xattr.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index 7c39157..81ec1c6 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -156,11 +156,12 @@ ext2_xattr_get(struct inode *inode, int
> name_index, const char *name,
> char *end;
> int error;
>
> + if (name == NULL)
> + return -EINVAL;
> +
> ea_idebug(inode, "name=%d.%s, buffer=%p, buffer_size=%ld",
> name_index, name, buffer, (long)buffer_size);
>
> - if (name == NULL)
> - return -EINVAL;
> down_read(&EXT2_I(inode)->xattr_sem);
> error = -ENODATA;
> if (!EXT2_I(inode)->i_file_acl)
> --
> 1.6.3.3
>
>
>
>
> --
>
>
> Thanks and Best Regards,
> shenghui
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo(a)vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: shenghui on 10 Jul 2010 20:40

---

Thanks for your explanation!

--


Thanks and Best Regards,
shenghui
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

From: shenghui on 11 Jul 2010 04:40

---

2010/7/11 crosslonelyover <crosslonelyover(a)gmail.com>:
> Hi Dan Carpenter,
>
>     Thanks for your instructions. I regenerated the patch.
> Please check it.
>
> From 3149865e0d118b939c27659a4e4c884eaa6b3a4c Mon Sep 17 00:00:00 2001
> From: Wang Sheng-Hui <crosslonelyover(a)gmail.com>
> Date: Sat, 10 Jul 2010 22:07:37 -0400
> Subject: [PATCH] avoid NULL deference in ext2_xattr_get
>
>
> Signed-off-by: Wang Sheng-Hui <crosslonelyover(a)gmail.com>
> ---
>  fs/ext2/xattr.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
> index 7c39157..81ec1c6 100644
> --- a/fs/ext2/xattr.c
> +++ b/fs/ext2/xattr.c
> @@ -156,11 +156,12 @@ ext2_xattr_get(struct inode *inode, int name_index, const char *name,
>        char *end;
>        int error;
>
> +       if (name == NULL)
> +               return -EINVAL;
> +
>        ea_idebug(inode, "name=%d.%s, buffer=%p, buffer_size=%ld",
>                  name_index, name, buffer, (long)buffer_size);
>
> -       if (name == NULL)
> -               return -EINVAL;
>        down_read(&EXT2_I(inode)->xattr_sem);
>        error = -ENODATA;
>        if (!EXT2_I(inode)->i_file_acl)
> --
> 1.7.1.1
>
>
>

Please ignore this patch. Just for gmail format check.
Thanks for David Beal's explanation!

--


Thanks and Best Regards,
shenghui
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

 | 
Pages: 1
Prev: parser: define __builtin_unreachable
Next: [PATCH] KVM: PIT: stop vpit before irq_routing freed