Re: [PATCH] atm/clip: Use seq_puts() in svc_addr()

[SF Markus Elfring]

>> @@ -708,11 +708,11 @@ static void svc_addr(struct seq_file *seq, struct 
>> sockaddr_atmsvc *addr)
>> static int e164[] = { 1, 8, 4, 6, 1, 0 };
>>
>> if (*addr->sas_addr.pub) {
>> -   seq_printf(seq, "%s", addr->sas_addr.pub);
>> +   seq_puts(seq, addr->sas_addr.pub);
> 
> Which opens a lot of security concerns.

How? - The passed string is just copied into a buffer finally, isn't it?


> Never do this again.

Why do you not like such a small source code transformation at the moment?


> P.S. I'm wondering what would be first,

I am curious on how communication difficulties can be adjusted.


> Markus starts looking into the actual code,

I inspected the original source code to some degree.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/fs/seq_file.c?id=895c0dde398510a5b5ded60e5064c11b94bd30ca#n682
https://elixir.free-electrons.com/linux/v4.15-rc6/source/fs/seq_file.c#L660


> or most (all) of the maintainers just ban him.

The change acceptance is varying for various reasons by the involved 
contributors.

Regards,
Markus

Re: [PATCH] atm/clip: Use seq_puts() in svc_addr()

[SF Markus Elfring]

>> @@ -708,11 +708,11 @@ static void svc_addr(struct seq_file *seq, struct 
>> sockaddr_atmsvc *addr)
>> static int e164[] = { 1, 8, 4, 6, 1, 0 };
>>
>> if (*addr->sas_addr.pub) {
>> -   seq_printf(seq, "%s", addr->sas_addr.pub);
>> +   seq_puts(seq, addr->sas_addr.pub);
> 
> Which opens a lot of security concerns.

How? - The passed string is just copied into a buffer finally, isn't it?


> Never do this again.

Why do you not like such a small source code transformation at the moment?


> P.S. I'm wondering what would be first,

I am curious on how communication difficulties can be adjusted.


> Markus starts looking into the actual code,

I inspected the original source code to some degree.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/fs/seq_file.c?id=895c0dde398510a5b5ded60e5064c11b94bd30ca#n682
https://elixir.free-electrons.com/linux/v4.15-rc6/source/fs/seq_file.c#L660


> or most (all) of the maintainers just ban him.

The change acceptance is varying for various reasons by the involved 
contributors.

Regards,
Markus

Re: [PATCH] atm/clip: Use seq_puts() in svc_addr()

[Andy Shevchenko]

On Sat, Jan 6, 2018 at 11:44 PM, SF Markus Elfring
 wrote:
> From: Markus Elfring 
> Date: Sat, 6 Jan 2018 22:34:12 +0100
>
> Two strings should be quickly put into a sequence by two function calls.
> Thus use the function "seq_puts" instead of "seq_printf".
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring 
> ---
>  net/atm/clip.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/atm/clip.c b/net/atm/clip.c
> index d4f6029d5109..62a852165b19 100644
> --- a/net/atm/clip.c
> +++ b/net/atm/clip.c
> @@ -708,11 +708,11 @@ static void svc_addr(struct seq_file *seq, struct 
> sockaddr_atmsvc *addr)
> static int e164[] = { 1, 8, 4, 6, 1, 0 };
>
> if (*addr->sas_addr.pub) {
> -   seq_printf(seq, "%s", addr->sas_addr.pub);
> +   seq_puts(seq, addr->sas_addr.pub);

Which opens a lot of security concerns.
Never do this again.

> if (*addr->sas_addr.prv)
> seq_putc(seq, '+');
> } else if (!*addr->sas_addr.prv) {

> -   seq_printf(seq, "%s", "(none)");
> +   seq_puts(seq, "(none)");

...while this one is okay per se, better to keep above pattern (same
style over the piece of code / function).

> return;
> }
> if (*addr->sas_addr.prv) {
> --
> 2.15.1
>

P.S. I'm wondering what would be first, Markus starts looking into the
actual code, or most (all) of the maintainers just ban him.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] atm/clip: Use seq_puts() in svc_addr()

[Andy Shevchenko]

On Sat, Jan 6, 2018 at 11:44 PM, SF Markus Elfring
 wrote:
> From: Markus Elfring 
> Date: Sat, 6 Jan 2018 22:34:12 +0100
>
> Two strings should be quickly put into a sequence by two function calls.
> Thus use the function "seq_puts" instead of "seq_printf".
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring 
> ---
>  net/atm/clip.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/atm/clip.c b/net/atm/clip.c
> index d4f6029d5109..62a852165b19 100644
> --- a/net/atm/clip.c
> +++ b/net/atm/clip.c
> @@ -708,11 +708,11 @@ static void svc_addr(struct seq_file *seq, struct 
> sockaddr_atmsvc *addr)
> static int e164[] = { 1, 8, 4, 6, 1, 0 };
>
> if (*addr->sas_addr.pub) {
> -   seq_printf(seq, "%s", addr->sas_addr.pub);
> +   seq_puts(seq, addr->sas_addr.pub);

Which opens a lot of security concerns.
Never do this again.

> if (*addr->sas_addr.prv)
> seq_putc(seq, '+');
> } else if (!*addr->sas_addr.prv) {

> -   seq_printf(seq, "%s", "(none)");
> +   seq_puts(seq, "(none)");

...while this one is okay per se, better to keep above pattern (same
style over the piece of code / function).

> return;
> }
> if (*addr->sas_addr.prv) {
> --
> 2.15.1
>

P.S. I'm wondering what would be first, Markus starts looking into the
actual code, or most (all) of the maintainers just ban him.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] atm/clip: Use seq_puts() in svc_addr()

[Stefano Brivio]

On Sat, 6 Jan 2018 22:44:08 +0100
SF Markus Elfring  wrote:

> From: Markus Elfring 
> Date: Sat, 6 Jan 2018 22:34:12 +0100
> 
> Two strings should be quickly put into a sequence by two function calls.
> Thus use the function "seq_puts" instead of "seq_printf".
>
> This issue was detected by using the Coccinelle software.

Can you please explain what the issue really is and what you're trying
to do here? One shouldn't need to dig into Coccinelle patterns to find
out what you mean, and "strings should be quickly put into a sequence"
isn't terribly helpful.

-- 
Stefano

Re: [PATCH] atm/clip: Use seq_puts() in svc_addr()

[Stefano Brivio]

On Sat, 6 Jan 2018 22:44:08 +0100
SF Markus Elfring  wrote:

> From: Markus Elfring 
> Date: Sat, 6 Jan 2018 22:34:12 +0100
> 
> Two strings should be quickly put into a sequence by two function calls.
> Thus use the function "seq_puts" instead of "seq_printf".
>
> This issue was detected by using the Coccinelle software.

Can you please explain what the issue really is and what you're trying
to do here? One shouldn't need to dig into Coccinelle patterns to find
out what you mean, and "strings should be quickly put into a sequence"
isn't terribly helpful.

-- 
Stefano