* Re: [patch] staging: alarm-dev: information leak in alarm_ioctl()
2013-06-03 9:02 [patch] staging: alarm-dev: information leak in alarm_ioctl() Dan Carpenter
@ 2013-06-03 19:08 ` John Stultz
2013-06-04 13:13 ` [patch] staging: alarm-dev: information leak in alarm_compat_ioctl() Dan Carpenter
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: John Stultz @ 2013-06-03 19:08 UTC (permalink / raw)
To: kernel-janitors
On 06/03/2013 02:02 AM, Dan Carpenter wrote:
> Smatch complains that if we pass an invalid clock type then "ts" is
> never set. We need to check for errors earlier, otherwise we end up
> passing uninitialized stack data to userspace.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Looks ok to me. Although you probably need the exact same change in the
compat_ioctl implementation?
Otherwise, Acked-by: John Stultz <john.stultz@linaro.org>
Cc'ing Android folks for their review as well.
thanks
-john
>
> diff --git a/drivers/staging/android/alarm-dev.c b/drivers/staging/android/alarm-dev.c
> index ceb1c643..c8600d9 100644
> --- a/drivers/staging/android/alarm-dev.c
> +++ b/drivers/staging/android/alarm-dev.c
> @@ -264,6 +264,8 @@ static long alarm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> }
>
> rv = alarm_do_ioctl(file, cmd, &ts);
> + if (rv)
> + return rv;
>
> switch (ANDROID_ALARM_BASE_CMD(cmd)) {
> case ANDROID_ALARM_GET_TIME(0):
> @@ -272,7 +274,7 @@ static long alarm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> break;
> }
>
> - return rv;
> + return 0;
> }
> #ifdef CONFIG_COMPAT
> static long alarm_compat_ioctl(struct file *file, unsigned int cmd,
^ permalink raw reply [flat|nested] 6+ messages in thread* [patch] staging: alarm-dev: information leak in alarm_compat_ioctl()
2013-06-03 9:02 [patch] staging: alarm-dev: information leak in alarm_ioctl() Dan Carpenter
2013-06-03 19:08 ` John Stultz
@ 2013-06-04 13:13 ` Dan Carpenter
2013-06-05 0:07 ` Arve Hjønnevåg
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2013-06-04 13:13 UTC (permalink / raw)
To: kernel-janitors
If we pass an invalid clock type then "ts" is never set. We need to
check for errors earlier, otherwise we end up passing uninitialized
stack data to userspace.
Reported-by: John Stultz <john.stultz@linaro.org>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/staging/android/alarm-dev.c b/drivers/staging/android/alarm-dev.c
index c8600d9..6dc27da 100644
--- a/drivers/staging/android/alarm-dev.c
+++ b/drivers/staging/android/alarm-dev.c
@@ -297,6 +297,8 @@ static long alarm_compat_ioctl(struct file *file, unsigned int cmd,
}
rv = alarm_do_ioctl(file, cmd, &ts);
+ if (rv)
+ return rv;
switch (ANDROID_ALARM_BASE_CMD(cmd)) {
case ANDROID_ALARM_GET_TIME(0): /* NOTE: we modified cmd above */
@@ -305,7 +307,7 @@ static long alarm_compat_ioctl(struct file *file, unsigned int cmd,
break;
}
- return rv;
+ return 0;
}
#endif
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [patch] staging: alarm-dev: information leak in alarm_compat_ioctl()
2013-06-03 9:02 [patch] staging: alarm-dev: information leak in alarm_ioctl() Dan Carpenter
2013-06-03 19:08 ` John Stultz
2013-06-04 13:13 ` [patch] staging: alarm-dev: information leak in alarm_compat_ioctl() Dan Carpenter
@ 2013-06-05 0:07 ` Arve Hjønnevåg
2013-06-05 0:25 ` John Stultz
2013-06-05 0:49 ` Arve Hjønnevåg
4 siblings, 0 replies; 6+ messages in thread
From: Arve Hjønnevåg @ 2013-06-05 0:07 UTC (permalink / raw)
To: kernel-janitors
On Tue, Jun 4, 2013 at 6:13 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> If we pass an invalid clock type then "ts" is never set. We need to
> check for errors earlier, otherwise we end up passing uninitialized
> stack data to userspace.
>
> Reported-by: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/staging/android/alarm-dev.c b/drivers/staging/android/alarm-dev.c
> index c8600d9..6dc27da 100644
> --- a/drivers/staging/android/alarm-dev.c
> +++ b/drivers/staging/android/alarm-dev.c
> @@ -297,6 +297,8 @@ static long alarm_compat_ioctl(struct file *file, unsigned int cmd,
> }
>
> rv = alarm_do_ioctl(file, cmd, &ts);
> + if (rv)
> + return rv;
>
> switch (ANDROID_ALARM_BASE_CMD(cmd)) {
> case ANDROID_ALARM_GET_TIME(0): /* NOTE: we modified cmd above */
> @@ -305,7 +307,7 @@ static long alarm_compat_ioctl(struct file *file, unsigned int cmd,
> break;
> }
>
> - return rv;
> + return 0;
> }
> #endif
>
Is there a separate fix for alarm_ioctl? It seems to have the same problem.
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] staging: alarm-dev: information leak in alarm_compat_ioctl()
2013-06-03 9:02 [patch] staging: alarm-dev: information leak in alarm_ioctl() Dan Carpenter
` (2 preceding siblings ...)
2013-06-05 0:07 ` Arve Hjønnevåg
@ 2013-06-05 0:25 ` John Stultz
2013-06-05 0:49 ` Arve Hjønnevåg
4 siblings, 0 replies; 6+ messages in thread
From: John Stultz @ 2013-06-05 0:25 UTC (permalink / raw)
To: kernel-janitors
On 06/04/2013 05:07 PM, Arve Hjønnevåg wrote:
> On Tue, Jun 4, 2013 at 6:13 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>> If we pass an invalid clock type then "ts" is never set. We need to
>> check for errors earlier, otherwise we end up passing uninitialized
>> stack data to userspace.
>>
>> Reported-by: John Stultz <john.stultz@linaro.org>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>
>> diff --git a/drivers/staging/android/alarm-dev.c b/drivers/staging/android/alarm-dev.c
>> index c8600d9..6dc27da 100644
>> --- a/drivers/staging/android/alarm-dev.c
>> +++ b/drivers/staging/android/alarm-dev.c
>> @@ -297,6 +297,8 @@ static long alarm_compat_ioctl(struct file *file, unsigned int cmd,
>> }
>>
>> rv = alarm_do_ioctl(file, cmd, &ts);
>> + if (rv)
>> + return rv;
>>
>> switch (ANDROID_ALARM_BASE_CMD(cmd)) {
>> case ANDROID_ALARM_GET_TIME(0): /* NOTE: we modified cmd above */
>> @@ -305,7 +307,7 @@ static long alarm_compat_ioctl(struct file *file, unsigned int cmd,
>> break;
>> }
>>
>> - return rv;
>> + return 0;
>> }
>> #endif
>>
> Is there a separate fix for alarm_ioctl? It seems to have the same problem.
Yea, I CC'ed the kernel-team alias yesterday on Dan's original fix for
the alarm_ioctl, which Greg has already queued.
This is just the follow-on fix to catch the same issue (as you also
noted) in the compat_ioctl.
thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] staging: alarm-dev: information leak in alarm_compat_ioctl()
2013-06-03 9:02 [patch] staging: alarm-dev: information leak in alarm_ioctl() Dan Carpenter
` (3 preceding siblings ...)
2013-06-05 0:25 ` John Stultz
@ 2013-06-05 0:49 ` Arve Hjønnevåg
4 siblings, 0 replies; 6+ messages in thread
From: Arve Hjønnevåg @ 2013-06-05 0:49 UTC (permalink / raw)
To: kernel-janitors
On Tue, Jun 4, 2013 at 5:25 PM, John Stultz <john.stultz@linaro.org> wrote:
> On 06/04/2013 05:07 PM, Arve Hjønnevåg wrote:
>>
>> On Tue, Jun 4, 2013 at 6:13 AM, Dan Carpenter <dan.carpenter@oracle.com>
>> wrote:
>>>
>>> If we pass an invalid clock type then "ts" is never set. We need to
>>> check for errors earlier, otherwise we end up passing uninitialized
>>> stack data to userspace.
>>>
>>> Reported-by: John Stultz <john.stultz@linaro.org>
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> diff --git a/drivers/staging/android/alarm-dev.c
>>> b/drivers/staging/android/alarm-dev.c
>>> index c8600d9..6dc27da 100644
>>> --- a/drivers/staging/android/alarm-dev.c
>>> +++ b/drivers/staging/android/alarm-dev.c
>>> @@ -297,6 +297,8 @@ static long alarm_compat_ioctl(struct file *file,
>>> unsigned int cmd,
>>> }
>>>
>>> rv = alarm_do_ioctl(file, cmd, &ts);
>>> + if (rv)
>>> + return rv;
>>>
>>> switch (ANDROID_ALARM_BASE_CMD(cmd)) {
>>> case ANDROID_ALARM_GET_TIME(0): /* NOTE: we modified cmd above
>>> */
>>> @@ -305,7 +307,7 @@ static long alarm_compat_ioctl(struct file *file,
>>> unsigned int cmd,
>>> break;
>>> }
>>>
>>> - return rv;
>>> + return 0;
>>> }
>>> #endif
>>>
>> Is there a separate fix for alarm_ioctl? It seems to have the same
>> problem.
>
> Yea, I CC'ed the kernel-team alias yesterday on Dan's original fix for the
> alarm_ioctl, which Greg has already queued.
>
> This is just the follow-on fix to catch the same issue (as you also noted)
> in the compat_ioctl.
>
> thanks
> -john
>
Sorry, I missed that.
Acked-by: Arve Hjønnevåg <arve@android.com>
to both.
--
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread