* s390: defective uses of va_arg in __debug_sprintf_event
@ 2018-04-06 19:08 Joe Perches
2018-04-09 12:39 ` Martin Schwidefsky
2018-04-10 14:35 ` Petr Mladek
0 siblings, 2 replies; 4+ messages in thread
From: Joe Perches @ 2018-04-06 19:08 UTC (permalink / raw)
To: Martin Schwidefsky, Heiko Carstens
Cc: linux-s390, LKML, Rasmus Villemoes, Petr Mladek
debug_sprintf_event calls __debug_sprintf_event
with a format and arguments.
There various types of arguments used in these
call, but __debug_sprintf_event uses va_arg
with only long as the type argument so random
errors could occur because the type and argument
are supposed to match.
debug_entry_t *__debug_sprintf_event(debug_info_t *id, int level, char *string, ...)
{
[...]
va_start(ap, string);
curr_event->string = string;
for (idx = 0; idx < min(numargs, (int)(id->buf_size / sizeof(long)) - 1); idx++)
curr_event->args[idx] = va_arg(ap, long);
va_end(ap);
[...]
}
from man va_arg
va_arg()
if type is not compatible with the type of the actual next argument
(as promoted according to the default argument promotions),
random errors will occur.
For instance, uses like:
arch/s390/kernel/perf_cpum_sf.c:919: debug_sprintf_event(sfdbg, 6, "pmu_enable: es=%i cs=%i ed=%i cd=%i "
arch/s390/kernel/perf_cpum_sf.c-920- "tear=%p dear=%p\n", cpuhw->lsctl.es, cpuhw->lsctl.cs,
arch/s390/kernel/perf_cpum_sf.c-921- cpuhw->lsctl.ed, cpuhw->lsctl.cd,
arch/s390/kernel/perf_cpum_sf.c-922- (void *) cpuhw->lsctl.tear, (void *) cpuhw->lsctl.dear);
where the first 3 arguments are int but their type
as used by va_arg in __debug_sprintf_event is long
which could produce random errors.
Instead of adding complete format % decoding,
perhaps the easiest solution is to change all
the formats to use %lu or %ld and cast each
argument as appropriate.
And I found this when looking at another defect
in debug_sprintf_event where a %p<foo> extension
is unintentionally used via a string concatenation.
as pointed out by Rasmus Villemoes
---
arch/s390/kernel/perf_cpum_sf.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/arch/s390/kernel/perf_cpum_sf.c b/arch/s390/kernel/perf_cpum_sf.c
index 1c9ddd7aa5ec..1c449a6f841a 100644
--- a/arch/s390/kernel/perf_cpum_sf.c
+++ b/arch/s390/kernel/perf_cpum_sf.c
@@ -212,9 +212,7 @@ static int realloc_sampling_buffer(struct sf_buffer *sfb,
* the sampling buffer origin.
*/
if (sfb->sdbt != get_next_sdbt(tail)) {
- debug_sprintf_event(sfdbg, 3, "realloc_sampling_buffer: "
- "sampling buffer is not linked: origin=%p"
- "tail=%p\n",
+ debug_sprintf_event(sfdbg, 3, "realloc_sampling_buffer: sampling buffer is not linked: origin=%p tail=%p\n",
(void *) sfb->sdbt, (void *) tail);
return -EINVAL;
}
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: s390: defective uses of va_arg in __debug_sprintf_event
2018-04-06 19:08 s390: defective uses of va_arg in __debug_sprintf_event Joe Perches
@ 2018-04-09 12:39 ` Martin Schwidefsky
2018-04-09 14:38 ` Joe Perches
2018-04-10 14:35 ` Petr Mladek
1 sibling, 1 reply; 4+ messages in thread
From: Martin Schwidefsky @ 2018-04-09 12:39 UTC (permalink / raw)
To: Joe Perches
Cc: Heiko Carstens, linux-s390, LKML, Rasmus Villemoes, Petr Mladek
On Fri, 06 Apr 2018 12:08:43 -0700
Joe Perches <joe@perches.com> wrote:
> debug_sprintf_event calls __debug_sprintf_event
> with a format and arguments.
>
> There various types of arguments used in these
> call, but __debug_sprintf_event uses va_arg
> with only long as the type argument so random
> errors could occur because the type and argument
> are supposed to match.
>
> debug_entry_t *__debug_sprintf_event(debug_info_t *id, int level, char *string, ...)
> {
> [...]
> va_start(ap, string);
> curr_event->string = string;
> for (idx = 0; idx < min(numargs, (int)(id->buf_size / sizeof(long)) - 1); idx++)
> curr_event->args[idx] = va_arg(ap, long);
> va_end(ap);
> [...]
> }
>
> from man va_arg
>
> va_arg()
>
> if type is not compatible with the type of the actual next argument
> (as promoted according to the default argument promotions),
> random errors will occur.
>
> For instance, uses like:
>
> arch/s390/kernel/perf_cpum_sf.c:919: debug_sprintf_event(sfdbg, 6, "pmu_enable: es=%i cs=%i ed=%i cd=%i "
> arch/s390/kernel/perf_cpum_sf.c-920- "tear=%p dear=%p\n", cpuhw->lsctl.es, cpuhw->lsctl.cs,
> arch/s390/kernel/perf_cpum_sf.c-921- cpuhw->lsctl.ed, cpuhw->lsctl.cd,
> arch/s390/kernel/perf_cpum_sf.c-922- (void *) cpuhw->lsctl.tear, (void *) cpuhw->lsctl.dear);
>
> where the first 3 arguments are int but their type
> as used by va_arg in __debug_sprintf_event is long
> which could produce random errors.
In principle you are right that the va_arg handling is not 100%
correct. It works though because the C ABI for s390x requires
that arguments are sign- or zero-extended to 64 bits by the caller
of a function. This is true for values passed in registers and for
the variable argument list.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: s390: defective uses of va_arg in __debug_sprintf_event
2018-04-09 12:39 ` Martin Schwidefsky
@ 2018-04-09 14:38 ` Joe Perches
0 siblings, 0 replies; 4+ messages in thread
From: Joe Perches @ 2018-04-09 14:38 UTC (permalink / raw)
To: Martin Schwidefsky
Cc: Heiko Carstens, linux-s390, LKML, Rasmus Villemoes, Petr Mladek
On Mon, 2018-04-09 at 14:39 +0200, Martin Schwidefsky wrote:
> On Fri, 06 Apr 2018 12:08:43 -0700
> Joe Perches <joe@perches.com> wrote:
>
> > debug_sprintf_event calls __debug_sprintf_event
> > with a format and arguments.
> >
> > There various types of arguments used in these
> > call, but __debug_sprintf_event uses va_arg
> > with only long as the type argument so random
> > errors could occur because the type and argument
> > are supposed to match.
> >
> > debug_entry_t *__debug_sprintf_event(debug_info_t *id, int level, char *string, ...)
> > {
> > [...]
> > va_start(ap, string);
> > curr_event->string = string;
> > for (idx = 0; idx < min(numargs, (int)(id->buf_size / sizeof(long)) - 1); idx++)
> > curr_event->args[idx] = va_arg(ap, long);
> > va_end(ap);
> > [...]
> > }
> >
> > from man va_arg
> >
> > va_arg()
> >
> > if type is not compatible with the type of the actual next argument
> > (as promoted according to the default argument promotions),
> > random errors will occur.
> >
> > For instance, uses like:
> >
> > arch/s390/kernel/perf_cpum_sf.c:919: debug_sprintf_event(sfdbg, 6, "pmu_enable: es=%i cs=%i ed=%i cd=%i "
> > arch/s390/kernel/perf_cpum_sf.c-920- "tear=%p dear=%p\n", cpuhw->lsctl.es, cpuhw->lsctl.cs,
> > arch/s390/kernel/perf_cpum_sf.c-921- cpuhw->lsctl.ed, cpuhw->lsctl.cd,
> > arch/s390/kernel/perf_cpum_sf.c-922- (void *) cpuhw->lsctl.tear, (void *) cpuhw->lsctl.dear);
> >
> > where the first 3 arguments are int but their type
> > as used by va_arg in __debug_sprintf_event is long
> > which could produce random errors.
>
> In principle you are right that the va_arg handling is not 100%
> correct. It works though because the C ABI for s390x requires
> that arguments are sign- or zero-extended to 64 bits by the caller
> of a function. This is true for values passed in registers and for
> the variable argument list.
Thanks.
Reference:
http://legacy.redhat.com/pub/redhat/linux/7.1/es/os/s390x/doc/lzsabi0.pdf
(found via your explanation)
It might be nice to explain that somewhere in
Documentation/s390 if it's not there already.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: s390: defective uses of va_arg in __debug_sprintf_event
2018-04-06 19:08 s390: defective uses of va_arg in __debug_sprintf_event Joe Perches
2018-04-09 12:39 ` Martin Schwidefsky
@ 2018-04-10 14:35 ` Petr Mladek
1 sibling, 0 replies; 4+ messages in thread
From: Petr Mladek @ 2018-04-10 14:35 UTC (permalink / raw)
To: Joe Perches
Cc: Martin Schwidefsky, Heiko Carstens, linux-s390, LKML, Rasmus Villemoes
On Fri 2018-04-06 12:08:43, Joe Perches wrote:
> And I found this when looking at another defect
> in debug_sprintf_event where a %p<foo> extension
> is unintentionally used via a string concatenation.
> as pointed out by Rasmus Villemoes
> ---
> arch/s390/kernel/perf_cpum_sf.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/arch/s390/kernel/perf_cpum_sf.c b/arch/s390/kernel/perf_cpum_sf.c
> index 1c9ddd7aa5ec..1c449a6f841a 100644
> --- a/arch/s390/kernel/perf_cpum_sf.c
> +++ b/arch/s390/kernel/perf_cpum_sf.c
> @@ -212,9 +212,7 @@ static int realloc_sampling_buffer(struct sf_buffer *sfb,
> * the sampling buffer origin.
> */
> if (sfb->sdbt != get_next_sdbt(tail)) {
> - debug_sprintf_event(sfdbg, 3, "realloc_sampling_buffer: "
> - "sampling buffer is not linked: origin=%p"
> - "tail=%p\n",
> + debug_sprintf_event(sfdbg, 3, "realloc_sampling_buffer: sampling buffer is not linked: origin=%p tail=%p\n",
> (void *) sfb->sdbt, (void *) tail);
> return -EINVAL;
> }
This concatenation fix is correct. %p has many possible specifiers.
The code takes a simple approach and substitute "%ptail" with the
pointer value. See the following code in vsnprintf():
case FORMAT_TYPE_PTR:
str = pointer(fmt, str, end, va_arg(args, void *),
spec);
while (isalnum(*fmt))
fmt++;
break;
We might debate if vsprintf() should handle this better. Anyway, there
is a missing space in the above debug_sprintf_event() call.
Joe, are you going to send this as a separate patch, your SOB, ...?
Best Regards,
Petr
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-04-10 14:36 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-06 19:08 s390: defective uses of va_arg in __debug_sprintf_event Joe Perches
2018-04-09 12:39 ` Martin Schwidefsky
2018-04-09 14:38 ` Joe Perches
2018-04-10 14:35 ` Petr Mladek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.