[Toybox] Integration of SMACK

[Rob Landley]

On 04/30/2015 07:49 AM, José Bollo wrote:
> Le jeudi 30 avril 2015 à 00:15 -0500, Rob Landley a écrit :
>>
>> I mentioned the redundant path creation and switching it out for openat(O_PATH)
>> and fgetxattr(). (If smack vetoes this open in ls, smack is probably going to
>> have problems with other programs using modern apis.)
> 
> Hello Rob,
> 
> Thanks a lot for all of your patience and good work. I checked your
> patch (despite that I didn't seen the starting point). That is great.
> Bravo.
> 
> Of course I changed things but very little.
> 
> There is however something bad. I wanted to use O_NOATIME but found that
> it is not defined.

According to git annotate include/uapi/asm-generic/fcntl.h it's been
there since at least 2005 (and that was consolidating it from the
platform-specific headers)...

Ah:
https://git.kernel.org/cgit/linux/kernel/git/tglx/history.git/commit/?id=5a86174c17ab9d62

June 20, 2004.

That said, Posix doesn't seem to have caught up yet:

http://pubs.opengroup.org/onlinepubs/9699919799//functions/open.html

> The issue can be solved by defining _GNU_SOURCE at
> the beginning of ls.c

To quote the 11th Doctor (in his case with regards to just walking past
a Fez), "Never gonna happen."

It looks like I should add a code.html explanation of my objection to
GNU_SOURCE. Many moons ago we had a discussion about this:

http://lists.landley.net/pipermail/toybox-landley.net/2012-March/002002.html

And I tried #defining a constellation of feature test macros to placate
the compiler into defining the symbols that we #included headers to get,
and it basically didn't work:

http://lists.landley.net/pipermail/toybox-landley.net/2012-March/002064.html

And I came to the conclusion that the proper way to fix this nonsense is
to stick it in lib/portability.h:

https://github.com/landley/toybox/blob/master/lib/portability.h#L219

I could alternately #include <asm-generic/fcntl.h> but that's not an
improvement.

> Halas it fails because of basename redefinition.
> So I added the defining of O_NOATIME in portability.

Indeed.

My pending patch added O_PATH there too. (Oddly that's in my
asm-generic/fcntl.h in ubuntu 2.12, but not in /usr/include/fcntl.h.)

I was going to use musl as an example of how to to do this sanely, but
for some inexplicable reason musl sticks O_NOATIME in each platform's
bits/fcntl.h despite the fact it's the same value on every single target
_and_ the kernel headers have had it in asm-generic since 2005. O_PATH
is in there too (with the same value on all targets).

*shrug*. We have portability.h.

>> The first -Z hunk used lgetxattr() but the second used getxattr(). So we
>> measured the length of the symlink attribute, then printed the file attribute?
>> I'm consistently using openat(O_PATH|O_NOFOLLOW) which should give me a
>> filehandle to the symlink...
>>
>> We're testing whether getxattr() returns a length > SMACK_LABEL_LEN. If that
>> can happen, did it stomp stack data outside the buffer? If it can't happen,
>> why are we testing for it?
> 
> Ooops that is my fault. IIRC, I saw that getxattr was used where
> lgetxattr is to be used and began to change it. But I finished not. And
> in fact the behaviour has to follow the -L option. But doing that is
> producing unexpected result that I should investigate.

openat(O_PATH) seems to follow symlinks by default, and not do so when
you feed it the O_NOFOLLOW bit. So that's easy enough to control...

>> Query: we feed sizeof(zlen) to getxattr(), I.E. SMACK_LABEL_LEN+1. But we
>> write a nul terminator, so the getxattr() call isn't terminating the buffer,
>> so why are we feeding it a buffer size _including_ space for a null terminator?
>>
>> The third -Z hunk is identical to the second -Z hunk except it's left
>> justified instead of right justified. Also your length is zlen-1 in hunk 3
>> and zlen without the -1 in hunk 2? Let's see... what you want is always one
>> space on the right, only a space on the left for -long, and yes the left/right
>> justification changes between -C and -l for no apparent reason but that's
>> what you expect so... (Note: sprintf %*s will left justify if fed a negative
>> length. Posix 2008 says so.)
> 
> Yes true. We are all expecting that "if (CFG_LS_SMACK)" fails then the
> code is removed by the compiler. So creating a function that is never
> called is ... Well it's ok

We compile with -ffunction-sections -fdata-sections and then feed the
linker --gc-sections. This means that each function winds up in its own
elf section and the unused elf sections are garbage collected at link time.

(Presumably newer compilers have a less awkward way of doing this, but
it's worked for 10 years now, so...)

This used to trigger a glibc bug back when Ulrich DrPepper was
maintainer, which he blamed on the linker people. Denys Vlasenko (currnt
busybox maintainer) gave a presentation about this topic in 2010:

http://free-electrons.com/blog/elc-2010-videos/

>> Why are you typecasting zlen to (int)? Doesn't subtracting a ssize_t work?
> 
> On 64 bits arch, ssize_t is 64 bits and int is 32 bits. Does it explain?

So it does the math in a 64 bit register and then converts it down
automatically when the resulting argument is of type int?

No, I still don't understand what the typecast accomplishes.

>> The query is setting a length of 0 if getxattr() doesn't find any data,
>> but you're printing "?" and a space, meaning the column sizing logic circa
>> line 550 needs adjusting too. (totpad += totals[7]+!!totals[7])
> 
>>From what I sew of your new implementation, the way how the spaces
> between fields is counted is mysterious. From my tests, it is not
> working because it is not counted.

Indeed, that would be the "I couldn't test this" part.

My goal was that the format field is " %*s" and we add 1 to that if we
don't want that space on the left, then the * is an integer argument
which is size to pad the field to, ala:

  #include <stdio.h>

  int main(int argc, char *argv[])
  {
    printf("%*s|\n", 10, "hello");
    printf("%*s|\n", -10, "hello");
  }

Produces:

       hello|
  hello     |

(In this case two space indentation of examples confuses matters
slightly, but hopefully not too much. :)

>> Alright, here's a wild guess at a patch, which I can't test because
>> the tizen image I installed following the instructions on
>> https://wiki.tizen.org/wiki/Tizen_Standalone_Emulator hasn't got a native
>> toolchain.
> 
> (snip)
> 
> I'm working on your proposal and will soon produce a submission. The
> current difficulty is that calling "dirtree_parentfd(dt)" is currently
> not reliable.

If so that's a bug and needs fixing. It should always produce either the
directory file descriptor or AT_FDCWD if we're at the top of the tree
(dt->parent is NULL). So you should always be able to feed that into the
first argument of openat().

> I'm still working on... Result in few minutes.

Ah, I have more mail.

Rob

 1430411560.0