From: Julia Lawall <[email protected]>
sizeof(viafb_gamma_table) is just the size of the pointer. This is changed
to the size used when calling kmalloc to initialize the pointer.
A simplified version of the semantic patch that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
// <smpl>
@@
expression *x;
expression f;
type T;
@@
*f(...,(T)x,...)
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/video/via/viafbdev.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/via/viafbdev.c b/drivers/video/via/viafbdev.c
index 56ec696..a0b47f1 100644
--- a/drivers/video/via/viafbdev.c
+++ b/drivers/video/via/viafbdev.c
@@ -680,7 +680,7 @@ static int viafb_ioctl(struct fb_info *info, u_int cmd, u_long arg)
if (!viafb_gamma_table)
return -ENOMEM;
if (copy_from_user(viafb_gamma_table, argp,
- sizeof(viafb_gamma_table))) {
+ 256 * sizeof(u32))) {
kfree(viafb_gamma_table);
return -EFAULT;
}
@@ -694,7 +694,7 @@ static int viafb_ioctl(struct fb_info *info, u_int cmd, u_long arg)
return -ENOMEM;
viafb_get_gamma_table(viafb_gamma_table);
if (copy_to_user(argp, viafb_gamma_table,
- sizeof(viafb_gamma_table))) {
+ 256 * sizeof(u32))) {
kfree(viafb_gamma_table);
return -EFAULT;
}
Hi Julia, Andrew,
Julia Lawall schrieb:
> From: Julia Lawall <[email protected]>
>
> sizeof(viafb_gamma_table) is just the size of the pointer. This is changed
> to the size used when calling kmalloc to initialize the pointer.
this is the second patch addressing this issue ending up in my mailbox.
At least this one is technically correct so feel free to upstream it.
However I vote for removing this ioctl hell from viafb as most of them
duplicate framebuffer functionality or have unknown (not clearly
defined) functionality or at least solve a generic problem with a custom
ioctl (which I consider bad). I had a patch ready to move this stuff to
an extra file and print a warning that it is subject to be removed. I
feel a bit uncomfortable about repairing broken stuff prior to removing it.
Any comments on this subject?
Thanks,
Florian Tobias Schandinat
> Signed-off-by: Julia Lawall <[email protected]>
>
> ---
> drivers/video/via/viafbdev.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/via/viafbdev.c b/drivers/video/via/viafbdev.c
> index 56ec696..a0b47f1 100644
> --- a/drivers/video/via/viafbdev.c
> +++ b/drivers/video/via/viafbdev.c
> @@ -680,7 +680,7 @@ static int viafb_ioctl(struct fb_info *info, u_int cmd, u_long arg)
> if (!viafb_gamma_table)
> return -ENOMEM;
> if (copy_from_user(viafb_gamma_table, argp,
> - sizeof(viafb_gamma_table))) {
> + 256 * sizeof(u32))) {
> kfree(viafb_gamma_table);
> return -EFAULT;
> }
> @@ -694,7 +694,7 @@ static int viafb_ioctl(struct fb_info *info, u_int cmd, u_long arg)
> return -ENOMEM;
> viafb_get_gamma_table(viafb_gamma_table);
> if (copy_to_user(argp, viafb_gamma_table,
> - sizeof(viafb_gamma_table))) {
> + 256 * sizeof(u32))) {
> kfree(viafb_gamma_table);
> return -EFAULT;
> }
On Sun, 13 Dec 2009 13:52:59 +0100 Florian Tobias Schandinat <[email protected]> wrote:
> Hi Julia, Andrew,
>
> Julia Lawall schrieb:
> > From: Julia Lawall <[email protected]>
> >
> > sizeof(viafb_gamma_table) is just the size of the pointer. This is changed
> > to the size used when calling kmalloc to initialize the pointer.
>
> this is the second patch addressing this issue ending up in my mailbox.
> At least this one is technically correct so feel free to upstream it.
> However I vote for removing this ioctl hell from viafb as most of them
> duplicate framebuffer functionality or have unknown (not clearly
> defined) functionality or at least solve a generic problem with a custom
> ioctl (which I consider bad). I had a patch ready to move this stuff to
> an extra file and print a warning that it is subject to be removed. I
> feel a bit uncomfortable about repairing broken stuff prior to removing it.
> Any comments on this subject?
>
I favour both repairing and removing broken stuff ;)
We may as well fix it if problems are known. Perhaps someone is
hitting the problem at runtime in an older kernel and needs a patch to
backport. Perhaps we later decide to revert the removal, thus
reinstating the known bug.
Hi Andrew,
thanks for your comment (and for taking the patch).
You can count it as Acked-by:me as it is technically correct. I also
tested it but as I don't know any program that uses this interface (and
not wanting to write my own as I do not want to encourage anyone to use
this interface) I can't say whether it now works as intended.
However this patch is certainly a step in the right direction as writing
random numbers to hardware is rarely correct :-)
Andrew Morton schrieb:
> On Sun, 13 Dec 2009 13:52:59 +0100 Florian Tobias Schandinat <[email protected]> wrote:
>
>> Hi Julia, Andrew,
>>
>> Julia Lawall schrieb:
>>> From: Julia Lawall <[email protected]>
>>>
>>> sizeof(viafb_gamma_table) is just the size of the pointer. This is changed
>>> to the size used when calling kmalloc to initialize the pointer.
>> this is the second patch addressing this issue ending up in my mailbox.
>> At least this one is technically correct so feel free to upstream it.
>> However I vote for removing this ioctl hell from viafb as most of them
>> duplicate framebuffer functionality or have unknown (not clearly
>> defined) functionality or at least solve a generic problem with a custom
>> ioctl (which I consider bad). I had a patch ready to move this stuff to
>> an extra file and print a warning that it is subject to be removed. I
>> feel a bit uncomfortable about repairing broken stuff prior to removing it.
>> Any comments on this subject?
>>
>
> I favour both repairing and removing broken stuff ;)
>
> We may as well fix it if problems are known. Perhaps someone is
> hitting the problem at runtime in an older kernel and needs a patch to
> backport. Perhaps we later decide to revert the removal, thus
> reinstating the known bug.
You are right.
I'll try to send a patch that at least labels these ioctls
unstable/depreciated as soon as time permits so that nobody will start
using them.
Regards,
Florian Tobias Schandinat