From: Colin Ian King <[email protected]>
Various usif_ioctl helper functions do not initialize the
return variable ret and some of the error handling return
paths just return garbage values that were on the stack (or
in a register). I believe that in all the cases, the
initial ret variable should be set to -EINVAL and subsequent
paths through these helper functions set it appropriately
otherwise.
Found via static analysis using cppcheck:
[drivers/gpu/drm/nouveau/nouveau_usif.c:138]:
(error) Uninitialized variable: ret
[drivers/gpu/drm/nouveau/nouveau_usif.c:179]:
(error) Uninitialized variable: ret
[drivers/gpu/drm/nouveau/nouveau_usif.c:202]:
(error) Uninitialized variable: ret
[drivers/gpu/drm/nouveau/nouveau_usif.c:241]:
(error) Uninitialized variable: ret
[drivers/gpu/drm/nouveau/nouveau_usif.c:157]:
(error) Uninitialized variable: ret
[drivers/gpu/drm/nouveau/nouveau_usif.c:288]:
(error) Uninitialized variable: ret
Signed-off-by: Colin Ian King <[email protected]>
---
drivers/gpu/drm/nouveau/nouveau_usif.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_usif.c b/drivers/gpu/drm/nouveau/nouveau_usif.c
index cb1182d..01b50a2 100644
--- a/drivers/gpu/drm/nouveau/nouveau_usif.c
+++ b/drivers/gpu/drm/nouveau/nouveau_usif.c
@@ -129,7 +129,7 @@ usif_notify_new(struct drm_file *f, void *data, u32 size, void *argv, u32 argc)
struct nvif_notify_req_v0 v0;
} *req;
struct usif_notify *ntfy;
- int ret;
+ int ret = -EINVAL;
if (nvif_unpack(args->v0, 0, 0, true)) {
if (usif_notify_find(f, args->v0.index))
@@ -170,7 +170,7 @@ usif_notify_del(struct drm_file *f, void *data, u32 size, void *argv, u32 argc)
struct nvif_ioctl_ntfy_del_v0 v0;
} *args = data;
struct usif_notify *ntfy;
- int ret;
+ int ret = -EINVAL;
if (nvif_unpack(args->v0, 0, 0, true)) {
if (!(ntfy = usif_notify_find(f, args->v0.index)))
@@ -193,7 +193,7 @@ usif_notify_get(struct drm_file *f, void *data, u32 size, void *argv, u32 argc)
struct nvif_ioctl_ntfy_del_v0 v0;
} *args = data;
struct usif_notify *ntfy;
- int ret;
+ int ret = -EINVAL;
if (nvif_unpack(args->v0, 0, 0, true)) {
if (!(ntfy = usif_notify_find(f, args->v0.index)))
@@ -232,7 +232,7 @@ usif_notify_put(struct drm_file *f, void *data, u32 size, void *argv, u32 argc)
struct nvif_ioctl_ntfy_put_v0 v0;
} *args = data;
struct usif_notify *ntfy;
- int ret;
+ int ret = -EINVAL;
if (nvif_unpack(args->v0, 0, 0, true)) {
if (!(ntfy = usif_notify_find(f, args->v0.index)))
@@ -269,7 +269,7 @@ usif_object_new(struct drm_file *f, void *data, u32 size, void *argv, u32 argc)
struct nvif_ioctl_new_v0 v0;
} *args = data;
struct usif_object *object;
- int ret;
+ int ret = -EINVAL;
if (!(object = kmalloc(sizeof(*object), GFP_KERNEL)))
return -ENOMEM;
--
2.1.4
On Wed, Jul 1, 2015 at 12:51 PM, Colin King <[email protected]> wrote:
> From: Colin Ian King <[email protected]>
>
> Various usif_ioctl helper functions do not initialize the
> return variable ret and some of the error handling return
> paths just return garbage values that were on the stack (or
> in a register). I believe that in all the cases, the
> initial ret variable should be set to -EINVAL and subsequent
> paths through these helper functions set it appropriately
> otherwise.
>
> Found via static analysis using cppcheck:
>
> [drivers/gpu/drm/nouveau/nouveau_usif.c:138]:
> (error) Uninitialized variable: ret
It sure would seem that way, wouldn't it?
#define nvif_unpack(d,vl,vh,m) ({ \
if ((vl) == 0 || ret == -ENOSYS) { \
int _size = sizeof(d); \
if (_size <= size && (d).version >= (vl) && \
(d).version <= (vh)) { \
data = (u8 *)data + _size; \
size = size - _size; \
ret = ((m) || !size) ? 0 : -E2BIG; \
} else { \
ret = -ENOSYS; \
} \
} \
(ret == 0); \
})
So actually it does get initialized, and I guess cppcheck doesn't know
about macros?
> [drivers/gpu/drm/nouveau/nouveau_usif.c:179]:
> (error) Uninitialized variable: ret
> [drivers/gpu/drm/nouveau/nouveau_usif.c:202]:
> (error) Uninitialized variable: ret
> [drivers/gpu/drm/nouveau/nouveau_usif.c:241]:
> (error) Uninitialized variable: ret
> [drivers/gpu/drm/nouveau/nouveau_usif.c:157]:
> (error) Uninitialized variable: ret
> [drivers/gpu/drm/nouveau/nouveau_usif.c:288]:
> (error) Uninitialized variable: ret
>
> Signed-off-by: Colin Ian King <[email protected]>
> ---
> drivers/gpu/drm/nouveau/nouveau_usif.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_usif.c b/drivers/gpu/drm/nouveau/nouveau_usif.c
> index cb1182d..01b50a2 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_usif.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_usif.c
> @@ -129,7 +129,7 @@ usif_notify_new(struct drm_file *f, void *data, u32 size, void *argv, u32 argc)
> struct nvif_notify_req_v0 v0;
> } *req;
> struct usif_notify *ntfy;
> - int ret;
> + int ret = -EINVAL;
>
> if (nvif_unpack(args->v0, 0, 0, true)) {
> if (usif_notify_find(f, args->v0.index))
> @@ -170,7 +170,7 @@ usif_notify_del(struct drm_file *f, void *data, u32 size, void *argv, u32 argc)
> struct nvif_ioctl_ntfy_del_v0 v0;
> } *args = data;
> struct usif_notify *ntfy;
> - int ret;
> + int ret = -EINVAL;
>
> if (nvif_unpack(args->v0, 0, 0, true)) {
> if (!(ntfy = usif_notify_find(f, args->v0.index)))
> @@ -193,7 +193,7 @@ usif_notify_get(struct drm_file *f, void *data, u32 size, void *argv, u32 argc)
> struct nvif_ioctl_ntfy_del_v0 v0;
> } *args = data;
> struct usif_notify *ntfy;
> - int ret;
> + int ret = -EINVAL;
>
> if (nvif_unpack(args->v0, 0, 0, true)) {
> if (!(ntfy = usif_notify_find(f, args->v0.index)))
> @@ -232,7 +232,7 @@ usif_notify_put(struct drm_file *f, void *data, u32 size, void *argv, u32 argc)
> struct nvif_ioctl_ntfy_put_v0 v0;
> } *args = data;
> struct usif_notify *ntfy;
> - int ret;
> + int ret = -EINVAL;
>
> if (nvif_unpack(args->v0, 0, 0, true)) {
> if (!(ntfy = usif_notify_find(f, args->v0.index)))
> @@ -269,7 +269,7 @@ usif_object_new(struct drm_file *f, void *data, u32 size, void *argv, u32 argc)
> struct nvif_ioctl_new_v0 v0;
> } *args = data;
> struct usif_object *object;
> - int ret;
> + int ret = -EINVAL;
>
> if (!(object = kmalloc(sizeof(*object), GFP_KERNEL)))
> return -ENOMEM;
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On 1 July 2015 at 17:56, Ilia Mirkin <[email protected]> wrote:
> On Wed, Jul 1, 2015 at 12:51 PM, Colin King <[email protected]> wrote:
>> From: Colin Ian King <[email protected]>
>>
>> Various usif_ioctl helper functions do not initialize the
>> return variable ret and some of the error handling return
>> paths just return garbage values that were on the stack (or
>> in a register). I believe that in all the cases, the
>> initial ret variable should be set to -EINVAL and subsequent
>> paths through these helper functions set it appropriately
>> otherwise.
>>
>> Found via static analysis using cppcheck:
>>
>> [drivers/gpu/drm/nouveau/nouveau_usif.c:138]:
>> (error) Uninitialized variable: ret
>
> It sure would seem that way, wouldn't it?
>
> #define nvif_unpack(d,vl,vh,m) ({ \
> if ((vl) == 0 || ret == -ENOSYS) { \
> int _size = sizeof(d); \
> if (_size <= size && (d).version >= (vl) && \
> (d).version <= (vh)) { \
> data = (u8 *)data + _size; \
> size = size - _size; \
> ret = ((m) || !size) ? 0 : -E2BIG; \
> } else { \
> ret = -ENOSYS; \
> } \
> } \
> (ret == 0); \
> })
>
> So actually it does get initialized, and I guess cppcheck doesn't know
> about macros?
>
I think I'm having deja-vu, but I do recall a similar mention to Ben.
Although in my defence I've assumed that nvif_unpack was a function,
as macros normally are normally all caps. Seems like the patch that
capitalises nvif_unpack never made it upstream :'-(
Cheers,
Emil
On 01/07/15 18:12, Emil Velikov wrote:
> On 1 July 2015 at 17:56, Ilia Mirkin <[email protected]> wrote:
>> On Wed, Jul 1, 2015 at 12:51 PM, Colin King <[email protected]> wrote:
>>> From: Colin Ian King <[email protected]>
>>>
>>> Various usif_ioctl helper functions do not initialize the
>>> return variable ret and some of the error handling return
>>> paths just return garbage values that were on the stack (or
>>> in a register). I believe that in all the cases, the
>>> initial ret variable should be set to -EINVAL and subsequent
>>> paths through these helper functions set it appropriately
>>> otherwise.
>>>
>>> Found via static analysis using cppcheck:
>>>
>>> [drivers/gpu/drm/nouveau/nouveau_usif.c:138]:
>>> (error) Uninitialized variable: ret
>>
>> It sure would seem that way, wouldn't it?
>>
>> #define nvif_unpack(d,vl,vh,m) ({ \
>> if ((vl) == 0 || ret == -ENOSYS) { \
>> int _size = sizeof(d); \
>> if (_size <= size && (d).version >= (vl) && \
>> (d).version <= (vh)) { \
>> data = (u8 *)data + _size; \
>> size = size - _size; \
>> ret = ((m) || !size) ? 0 : -E2BIG; \
>> } else { \
>> ret = -ENOSYS; \
>> } \
>> } \
>> (ret == 0); \
>> })
>>
>> So actually it does get initialized, and I guess cppcheck doesn't know
>> about macros?
Hrm, what about the case when ((vl) == 0 || ret == -ENOSYS) is false,
where is ret being set in that case?
>>
> I think I'm having deja-vu, but I do recall a similar mention to Ben.
> Although in my defence I've assumed that nvif_unpack was a function,
> as macros normally are normally all caps. Seems like the patch that
> capitalises nvif_unpack never made it upstream :'-(
>
> Cheers,
> Emil
>
On Wed, Jul 1, 2015 at 1:18 PM, Colin Ian King <[email protected]> wrote:
> On 01/07/15 18:12, Emil Velikov wrote:
>> On 1 July 2015 at 17:56, Ilia Mirkin <[email protected]> wrote:
>>> On Wed, Jul 1, 2015 at 12:51 PM, Colin King <[email protected]> wrote:
>>>> From: Colin Ian King <[email protected]>
>>>>
>>>> Various usif_ioctl helper functions do not initialize the
>>>> return variable ret and some of the error handling return
>>>> paths just return garbage values that were on the stack (or
>>>> in a register). I believe that in all the cases, the
>>>> initial ret variable should be set to -EINVAL and subsequent
>>>> paths through these helper functions set it appropriately
>>>> otherwise.
>>>>
>>>> Found via static analysis using cppcheck:
>>>>
>>>> [drivers/gpu/drm/nouveau/nouveau_usif.c:138]:
>>>> (error) Uninitialized variable: ret
>>>
>>> It sure would seem that way, wouldn't it?
>>>
>>> #define nvif_unpack(d,vl,vh,m) ({ \
>>> if ((vl) == 0 || ret == -ENOSYS) { \
>>> int _size = sizeof(d); \
>>> if (_size <= size && (d).version >= (vl) && \
>>> (d).version <= (vh)) { \
>>> data = (u8 *)data + _size; \
>>> size = size - _size; \
>>> ret = ((m) || !size) ? 0 : -E2BIG; \
>>> } else { \
>>> ret = -ENOSYS; \
>>> } \
>>> } \
>>> (ret == 0); \
>>> })
>>>
>>> So actually it does get initialized, and I guess cppcheck doesn't know
>>> about macros?
>
> Hrm, what about the case when ((vl) == 0 || ret == -ENOSYS) is false,
> where is ret being set in that case?
Is that actually the case for any of the callsites? gcc would complain
about that...
On 1 July 2015 at 18:37, Ilia Mirkin <[email protected]> wrote:
> On Wed, Jul 1, 2015 at 1:18 PM, Colin Ian King <[email protected]> wrote:
>> On 01/07/15 18:12, Emil Velikov wrote:
>>> On 1 July 2015 at 17:56, Ilia Mirkin <[email protected]> wrote:
>>>> On Wed, Jul 1, 2015 at 12:51 PM, Colin King <[email protected]> wrote:
>>>>> From: Colin Ian King <[email protected]>
>>>>>
>>>>> Various usif_ioctl helper functions do not initialize the
>>>>> return variable ret and some of the error handling return
>>>>> paths just return garbage values that were on the stack (or
>>>>> in a register). I believe that in all the cases, the
>>>>> initial ret variable should be set to -EINVAL and subsequent
>>>>> paths through these helper functions set it appropriately
>>>>> otherwise.
>>>>>
>>>>> Found via static analysis using cppcheck:
>>>>>
>>>>> [drivers/gpu/drm/nouveau/nouveau_usif.c:138]:
>>>>> (error) Uninitialized variable: ret
>>>>
>>>> It sure would seem that way, wouldn't it?
>>>>
>>>> #define nvif_unpack(d,vl,vh,m) ({ \
>>>> if ((vl) == 0 || ret == -ENOSYS) { \
>>>> int _size = sizeof(d); \
>>>> if (_size <= size && (d).version >= (vl) && \
>>>> (d).version <= (vh)) { \
>>>> data = (u8 *)data + _size; \
>>>> size = size - _size; \
>>>> ret = ((m) || !size) ? 0 : -E2BIG; \
>>>> } else { \
>>>> ret = -ENOSYS; \
>>>> } \
>>>> } \
>>>> (ret == 0); \
>>>> })
>>>>
>>>> So actually it does get initialized, and I guess cppcheck doesn't know
>>>> about macros?
>>
>> Hrm, what about the case when ((vl) == 0 || ret == -ENOSYS) is false,
>> where is ret being set in that case?
>
> Is that actually the case for any of the callsites? gcc would complain
> about that...
There is one:
drm/nouveau/nvkm/engine/disp/nv50.c: if (nvif_unpack(args->v1, 1, 1, true))
Seems like a recent addition though, I don't recall it with back when
was introduced.
-Emil
On Wed, Jul 1, 2015 at 1:59 PM, Emil Velikov <[email protected]> wrote:
> On 1 July 2015 at 18:37, Ilia Mirkin <[email protected]> wrote:
>> On Wed, Jul 1, 2015 at 1:18 PM, Colin Ian King <[email protected]> wrote:
>>> On 01/07/15 18:12, Emil Velikov wrote:
>>>> On 1 July 2015 at 17:56, Ilia Mirkin <[email protected]> wrote:
>>>>> On Wed, Jul 1, 2015 at 12:51 PM, Colin King <[email protected]> wrote:
>>>>>> From: Colin Ian King <[email protected]>
>>>>>>
>>>>>> Various usif_ioctl helper functions do not initialize the
>>>>>> return variable ret and some of the error handling return
>>>>>> paths just return garbage values that were on the stack (or
>>>>>> in a register). I believe that in all the cases, the
>>>>>> initial ret variable should be set to -EINVAL and subsequent
>>>>>> paths through these helper functions set it appropriately
>>>>>> otherwise.
>>>>>>
>>>>>> Found via static analysis using cppcheck:
>>>>>>
>>>>>> [drivers/gpu/drm/nouveau/nouveau_usif.c:138]:
>>>>>> (error) Uninitialized variable: ret
>>>>>
>>>>> It sure would seem that way, wouldn't it?
>>>>>
>>>>> #define nvif_unpack(d,vl,vh,m) ({ \
>>>>> if ((vl) == 0 || ret == -ENOSYS) { \
>>>>> int _size = sizeof(d); \
>>>>> if (_size <= size && (d).version >= (vl) && \
>>>>> (d).version <= (vh)) { \
>>>>> data = (u8 *)data + _size; \
>>>>> size = size - _size; \
>>>>> ret = ((m) || !size) ? 0 : -E2BIG; \
>>>>> } else { \
>>>>> ret = -ENOSYS; \
>>>>> } \
>>>>> } \
>>>>> (ret == 0); \
>>>>> })
>>>>>
>>>>> So actually it does get initialized, and I guess cppcheck doesn't know
>>>>> about macros?
>>>
>>> Hrm, what about the case when ((vl) == 0 || ret == -ENOSYS) is false,
>>> where is ret being set in that case?
>>
>> Is that actually the case for any of the callsites? gcc would complain
>> about that...
> There is one:
>
> drm/nouveau/nvkm/engine/disp/nv50.c: if (nvif_unpack(args->v1, 1, 1, true))
>
> Seems like a recent addition though, I don't recall it with back when
> was introduced.
It follows a call to nvif_unpack(0) though, which will initialize ret.