| Message ID | 87k2z86xvs.fsf@rasmusvillemoes.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2015-02-23 at 23:55 +0100, Rasmus Villemoes wrote: > On Mon, Feb 23 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > >> If you insist on a separate function for doing the overflow testing, > >> I'll just rip it out from my code and let you add such a test later. > > > > What about to make it a separate function *and* call from inside of > > test_string_escape? Would it work for you? > > See my earlier point about "quite a lot of state to pass". But if this > > static __init void > test_string_escape_overflow(const char *in, int p, char *out_real, int out_size, > unsigned int flags, const char *esc, int q_test, > const char *name) > { > int q_real; > > memset(out_real, 'Z', out_size); > q_real = string_escape_mem(in, p, out_real, 0, flags, esc); > if (q_real != q_test) > pr_warn("Test '%s' failed: flags = %u, osz = 0, expected %d, got %d\n", > name, flags, q_test, q_real); > if (memchr_inv(out_real, 'Z', out_size)) > pr_warn("Test '%s' failed: osz = 0 but string_escape_mem wrote to the buffer\n", > name); > } > > is what you want, sure, have it your way. Something like above, though might be few variables can be defined inside it, such as out_real, out_size. > I need to fix fs/proc/array.c in 3/3 as well, to make the kernel > compile+boot and make the series bisectable. Before I send v4 please let > me know what you think about this (the minimal fix I could come up with): > > diff --git a/fs/proc/array.c b/fs/proc/array.c > index 1295a00ca316..20f2d50e2dba 100644 > --- a/fs/proc/array.c > +++ b/fs/proc/array.c > @@ -99,10 +99,9 @@ static inline void task_name(struct seq_file *m, struct task_struct *p) > buf = m->buf + m->count; > > /* Ignore error for now */ > - string_escape_str(tcomm, &buf, m->size - m->count, > - ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\"); > + m->count += string_escape_str(tcomm, buf, m->size - m->count, > + ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\"); Just a nitpick: what if we keep buf arithmetics in place, i.e. buf += string_escape_str(); m->count = … Also shouldn't we check if seq_overflow is set before even trying to escape? Otherwise it will return something which is bigger that 0 and advance m->count too far. > > - m->count = buf - m->buf; > seq_putc(m, '\n'); > } > > [Longer-term I think it would be a lot better not to poke around in > the internals of struct seq_file. One way is to do the escaping into a > stack buffer (2*sizeof(p->comm) should be enough) and then use something > like seq_write(m, buffer, min(sizeof(buffer), > return-value-from-string_escape_str)). > > Another option is to do everything with a single seq_printf call, > something like > > seq_printf(m, "Name:\t%*pEcs\n, (int)strlen(tcomm), tcomm) > > That will escape more than just \ and \n, but that would IMO be an > improvement. But of course this is out of scope for this series.] It should be %pT and reconsider policy how we print task name in different cases (vsprintf.c::comm_name()).
On Mon, Mar 02 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Mon, 2015-02-23 at 23:55 +0100, Rasmus Villemoes wrote: >> On Mon, Feb 23 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > >> > What about to make it a separate function *and* call from inside of >> > test_string_escape? Would it work for you? >> >> See my earlier point about "quite a lot of state to pass". But if this >> >> static __init void >> test_string_escape_overflow(const char *in, int p, char *out_real, int out_size, >> unsigned int flags, const char *esc, int q_test, >> const char *name) >> { >> int q_real; >> >> memset(out_real, 'Z', out_size); >> q_real = string_escape_mem(in, p, out_real, 0, flags, esc); >> if (q_real != q_test) >> pr_warn("Test '%s' failed: flags = %u, osz = 0, expected %d, got %d\n", >> name, flags, q_test, q_real); >> if (memchr_inv(out_real, 'Z', out_size)) >> pr_warn("Test '%s' failed: osz = 0 but string_escape_mem wrote to the buffer\n", >> name); >> } >> >> is what you want, sure, have it your way. > > Something like above, though might be few variables can be defined > inside it, such as out_real, out_size. Or maybe not at all: We could pass NULL, 0, which is what has to work anyway for the kasprintf case - failure will then be detected through an oops, but I think that should be ok. That would also remove the memset and memchr_inv calls above. I don't like the idea of just defining a small stack buffer (say buf[16]) and passing that (still with a size of 0): It's better to either detect writes directly (by passing a large enough buffer with known contents), or indirectly through an oops, as opposed to having to figure it out from random stack corruption. And kmalloc'ing+error handling+kfree'ing a buffer inside the overflow check would just be plain silly, when we have a large enough buffer already. >> I need to fix fs/proc/array.c in 3/3 as well, to make the kernel >> compile+boot and make the series bisectable. Before I send v4 please let >> me know what you think about this (the minimal fix I could come up with): >> >> diff --git a/fs/proc/array.c b/fs/proc/array.c >> index 1295a00ca316..20f2d50e2dba 100644 >> --- a/fs/proc/array.c >> +++ b/fs/proc/array.c >> @@ -99,10 +99,9 @@ static inline void task_name(struct seq_file *m, struct task_struct *p) >> buf = m->buf + m->count; >> >> /* Ignore error for now */ >> - string_escape_str(tcomm, &buf, m->size - m->count, >> - ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\"); >> + m->count += string_escape_str(tcomm, buf, m->size - m->count, >> + ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\"); > > Just a nitpick: what if we keep buf arithmetics in place, i.e. > buf += string_escape_str(); > m->count = … Yes, that will make the patch even smaller, just touching that single line. Done. > Also shouldn't we check if seq_overflow is set before even trying to > escape? Otherwise it will return something which is bigger that 0 and > advance m->count too far. I don't think we need to care. AFAICT, task_name is only used for /proc/*/status, and it is the first thing to fill anything into the seq_file, so overflow is impossible. [Testing for pre-existing overflow also wouldn't be enough; one should check whether there's actually room for ~32 bytes.] As I said, I do think that longer-term one shouldn't have to poke around in the seq_file internals, but for now I'd like to make the patch minimal. >> Another option is to do everything with a single seq_printf call, >> something like >> >> seq_printf(m, "Name:\t%*pEcs\n, (int)strlen(tcomm), tcomm) >> >> That will escape more than just \ and \n, but that would IMO be an >> improvement. But of course this is out of scope for this series.] > > It should be %pT and reconsider policy how we print task name in > different cases (vsprintf.c::comm_name()). Well, %pT is a completely new addition to vsprintf.c. Also, I don't think that would be a very good match - not every user of %pT might want escaping, so at the very least this would require implementing some extra flags for %pT. But if task_name would be the only user of those flags, I think the escaping logic is better kept there. Anyway, this is outside this series' scope. Rasmus -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2015-03-03 at 00:03 +0100, Rasmus Villemoes wrote: > On Mon, Mar 02 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, 2015-02-23 at 23:55 +0100, Rasmus Villemoes wrote: > >> On Mon, Feb 23 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > >> > What about to make it a separate function *and* call from inside of > >> > test_string_escape? Would it work for you? > >> > >> See my earlier point about "quite a lot of state to pass". But if this > >> > >> static __init void > >> test_string_escape_overflow(const char *in, int p, char *out_real, int out_size, > >> unsigned int flags, const char *esc, int q_test, > >> const char *name) > >> { > >> int q_real; > >> > >> memset(out_real, 'Z', out_size); > >> q_real = string_escape_mem(in, p, out_real, 0, flags, esc); > >> if (q_real != q_test) > >> pr_warn("Test '%s' failed: flags = %u, osz = 0, expected %d, got %d\n", > >> name, flags, q_test, q_real); > >> if (memchr_inv(out_real, 'Z', out_size)) > >> pr_warn("Test '%s' failed: osz = 0 but string_escape_mem wrote to the buffer\n", > >> name); > >> } > >> > >> is what you want, sure, have it your way. > > > > Something like above, though might be few variables can be defined > > inside it, such as out_real, out_size. > > Or maybe not at all: We could pass NULL, 0, which is what has to work > anyway for the kasprintf case - failure will then be detected through an > oops, but I think that should be ok. That would also remove the memset and > memchr_inv calls above. > > I don't like the idea of just defining a small stack buffer (say > buf[16]) and passing that (still with a size of 0): It's better to > either detect writes directly (by passing a large enough buffer with > known contents), or indirectly through an oops, as opposed to having to > figure it out from random stack corruption. And kmalloc'ing+error > handling+kfree'ing a buffer inside the overflow check would just be > plain silly, when we have a large enough buffer already. Come with v4, I think I have no big objections to the approach. > As I said, I do think that longer-term one shouldn't have to poke around > in the seq_file internals, but for now I'd like to make the patch minimal. Ok. > >> Another option is to do everything with a single seq_printf call, > >> something like > >> > >> seq_printf(m, "Name:\t%*pEcs\n, (int)strlen(tcomm), tcomm) > >> > >> That will escape more than just \ and \n, but that would IMO be an > >> improvement. But of course this is out of scope for this series.] > > > > It should be %pT and reconsider policy how we print task name in > > different cases (vsprintf.c::comm_name()). > > Well, %pT is a completely new addition to vsprintf.c. Also, I don't > think that would be a very good match - not every user of %pT might want > escaping, so at the very least this would require implementing some > extra flags for %pT. Something like %pTe (for 'sanely Escaped' with flags you proposed earlier) ? > But if task_name would be the only user of those > flags, I think the escaping logic is better kept there. Anyway, this is > outside this series' scope. Yes.
diff --git a/fs/proc/array.c b/fs/proc/array.c index 1295a00ca316..20f2d50e2dba 100644 --- a/fs/proc/array.c +++ b/fs/proc/array.c @@ -99,10 +99,9 @@ static inline void task_name(struct seq_file *m, struct task_struct *p) buf = m->buf + m->count; /* Ignore error for now */ - string_escape_str(tcomm, &buf, m->size - m->count, - ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\"); + m->count += string_escape_str(tcomm, buf, m->size - m->count, + ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\"); - m->count = buf - m->buf; seq_putc(m, '\n'); }