[PATCH] memset_s() and talloc_set_secure()
abartlet at samba.org
Tue Mar 26 23:02:23 UTC 2019
On Tue, 2019-03-26 at 18:15 -0400, Simo wrote:
> I am not sure what is the concern ?
> there is a cost only for the case where you set the flag, for all other
> cases there should be no impact.
That has been the argument for every talloc feature added, and between
them talloc_free() has become quite costly.
> > I would want to see numbers to show this doesn't make the general case
> > slower: it turns out that unlikely() != zero cost, as we found out when
> > we optimised out some of the redundant magic checking.
> Are you concerned that 1 if statement will make a difference in
> practice ?
Yes. talloc_free() is a very hot path.
> > Also, each bit stolen from the magic makes the magic less secure as a
> > nonce/cookie defending against overwrite attacks.
> Sure, that's a good rule of thumb that we do not want to eat into flags
> for no good reason. But here we have a good reason.
If we do this, then my condition is that the flag is taken from
somewhere else, eg unifying the POOL flags.
> > > Being able to mark an existing talloc memory
> > > buffer as holding security sensitive data,
> > > and having it auto-zero'ed out on realloc/free does
> > > seem like an addition that will certainly
> > > be used (let's just make this a one-way
> > > gate to avoid extra complexity).
> > I'm still not convinced. To make some progress I've positively
> > reviewed the MR (now I have my head mostly back above water), my only
> > concern delaying me was naming (sorry!).
> What are you not convinced about exactly?
> That it will be used?
> It most certainly will be as Andreas plans to use the feature to mark
> keys being transported over talloc()'d memory.
It still seems, compared the the vast majority of talloc use that this
is a niche feature that can be implemented outside libtalloc using
existing features. We have found it incredibly difficult to remove
additional 'helpful' features from talloc and I'm keen to avoid it
where there is a practical alternative.
I also dislike the additional complexity in the talloc_realloc() case,
this is even more code for something that (statistically) just won't
I hope this clarifies things, and I'm sorry I didn't get the review tag
to Andreas's work earlier, as I've mentioned elsewhere I've been rather
Authentication Developer, Samba Team https://samba.org
Samba Development and Support, Catalyst IT
More information about the samba-technical