| Message ID | 1424956680-24758-1-git-send-email-anders.roxell@linaro.org |
|---|---|
| State | Accepted |
| Commit | d8b05a7e916738783651dd0e9aad0e31c32611e4 |
| Headers | show |
Thanks, applied. The same cast as in other places. Maxim. On 02/26/2015 04:18 PM, Anders Roxell wrote: > commit 4a4804a breaks on ARM 32-bit cross compile. > odp_crypto.c: In function 'odp_crypto_session_destroy': > odp_crypto.c:364:12: error: cast to pointer from integer of different > size [-Werror=int-to-pointer-cast] > generic = (odp_crypto_generic_session_t *)session; > > Signed-off-by: Anders Roxell <anders.roxell@linaro.org> > --- > platform/linux-generic/odp_crypto.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/platform/linux-generic/odp_crypto.c b/platform/linux-generic/odp_crypto.c > index 0c38263..2f13e2f 100644 > --- a/platform/linux-generic/odp_crypto.c > +++ b/platform/linux-generic/odp_crypto.c > @@ -361,7 +361,7 @@ int odp_crypto_session_destroy(odp_crypto_session_t session) > { > odp_crypto_generic_session_t *generic; > > - generic = (odp_crypto_generic_session_t *)session; > + generic = (odp_crypto_generic_session_t *)(intptr_t)session; > memset(generic, 0, sizeof(*generic)); > free_session(generic); > return 0;
On 26 February 2015 at 14:18, Anders Roxell <anders.roxell@linaro.org> wrote: > commit 4a4804a breaks on ARM 32-bit cross compile. > odp_crypto.c: In function 'odp_crypto_session_destroy': > odp_crypto.c:364:12: error: cast to pointer from integer of different > size [-Werror=int-to-pointer-cast] > generic = (odp_crypto_generic_session_t *)session; > > Signed-off-by: Anders Roxell <anders.roxell@linaro.org> > --- > platform/linux-generic/odp_crypto.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/platform/linux-generic/odp_crypto.c b/platform/linux-generic/odp_crypto.c > index 0c38263..2f13e2f 100644 > --- a/platform/linux-generic/odp_crypto.c > +++ b/platform/linux-generic/odp_crypto.c > @@ -361,7 +361,7 @@ int odp_crypto_session_destroy(odp_crypto_session_t session) > { > odp_crypto_generic_session_t *generic; > > - generic = (odp_crypto_generic_session_t *)session; > + generic = (odp_crypto_generic_session_t *)(intptr_t)session; > memset(generic, 0, sizeof(*generic)); Why are we clearing the session structure before the free (as I assume it is) below? Shouldn't wipe on free be a generic malloc/free (conditional debug) feature and not a mandatory part of some module that uses malloc? Is the crypto session somehow considered sensitive data (it doesn't contain any actual keys?) so we really want to wipe it as soon as it is no longer needed? > free_session(generic); > return 0; > -- > 2.1.4 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On 26 February 2015 at 18:52, Robbie King (robking) <robking@cisco.com> wrote: > When the memory is first allocated (at global_init), the entire memory > array is cleared. I memset it on free such that initial state is the > same at allocation time (whether it is the first allocation of the structure > or a subsequent). Alternatively it can be done at allocation, but I think > it should be done at some point before being (re)used. So free_session() doesn't give back the memory to the real allocator? To me it feels better to clear a data structure just after it has been allocated (and is going to be initialized and used), not when it is freed. To depend on initialization that happens elsewhere (when the whole memory array is allocated and when individual data structures are being freed) feels fragile. This dependency is not even documented (with a comment), at least not here when the data structure is cleared and freed. Someone might remove that memset (because it feels redundant) and then break an assumption made by code elsewhere. > > -----Original Message----- > From: Ola Liljedahl [mailto:ola.liljedahl@linaro.org] > Sent: Thursday, February 26, 2015 12:49 PM > To: Anders Roxell; Taras Kondratiuk; Robbie King (robking) > Cc: LNG ODP Mailman List > Subject: Re: [lng-odp] [PATCH] linux-generic: odp_crypto: use intptr_t to find the pointer > > On 26 February 2015 at 14:18, Anders Roxell <anders.roxell@linaro.org> wrote: >> commit 4a4804a breaks on ARM 32-bit cross compile. >> odp_crypto.c: In function 'odp_crypto_session_destroy': >> odp_crypto.c:364:12: error: cast to pointer from integer of different >> size [-Werror=int-to-pointer-cast] >> generic = (odp_crypto_generic_session_t *)session; >> >> Signed-off-by: Anders Roxell <anders.roxell@linaro.org> >> --- >> platform/linux-generic/odp_crypto.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/platform/linux-generic/odp_crypto.c b/platform/linux-generic/odp_crypto.c >> index 0c38263..2f13e2f 100644 >> --- a/platform/linux-generic/odp_crypto.c >> +++ b/platform/linux-generic/odp_crypto.c >> @@ -361,7 +361,7 @@ int odp_crypto_session_destroy(odp_crypto_session_t session) >> { >> odp_crypto_generic_session_t *generic; >> >> - generic = (odp_crypto_generic_session_t *)session; >> + generic = (odp_crypto_generic_session_t *)(intptr_t)session; >> memset(generic, 0, sizeof(*generic)); > Why are we clearing the session structure before the free (as I assume > it is) below? > Shouldn't wipe on free be a generic malloc/free (conditional debug) > feature and not a mandatory part of some module that uses malloc? > Is the crypto session somehow considered sensitive data (it doesn't > contain any actual keys?) so we really want to wipe it as soon as it > is no longer needed? > >> free_session(generic); >> return 0; >> -- >> 2.1.4 >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp
For cryptographic use, security folks also tend to be paranoid about "zeroizing" potentially sensitive data as soon as it's no longer needed. Recall we discussed having a zero-on-free option for buffers during the crypto sprint. The linux-generic pool code actually has this capability that we can activate when needed but it's disabled/dormant for now since it's not part of the v1.0 APIs. On Thu, Feb 26, 2015 at 1:20 PM, Robbie King (robking) <robking@cisco.com> wrote: > You are correct, "free_session" puts the session control structure > being returned back onto a free list maintained by crypto, it does > not return the shared memory back to the system. > > I have found clear/poison at free useful for catching stale pointers > in applications, where the pointer frees the structure but then > continues to access it. If it has been wiped or poisoned, there is > a better chance of catching this versus it staying valid (from the > application point of view) and then suddenly it's in use again > with different but valid fields. > > > -----Original Message----- > From: Ola Liljedahl [mailto:ola.liljedahl@linaro.org] > Sent: Thursday, February 26, 2015 2:14 PM > To: Robbie King (robking) > Cc: Anders Roxell; Taras Kondratiuk; LNG ODP Mailman List > Subject: Re: [lng-odp] [PATCH] linux-generic: odp_crypto: use intptr_t to > find the pointer > > On 26 February 2015 at 18:52, Robbie King (robking) <robking@cisco.com> > wrote: > > When the memory is first allocated (at global_init), the entire memory > > array is cleared. I memset it on free such that initial state is the > > same at allocation time (whether it is the first allocation of the > structure > > or a subsequent). Alternatively it can be done at allocation, but I > think > > it should be done at some point before being (re)used. > So free_session() doesn't give back the memory to the real allocator? > > To me it feels better to clear a data structure just after it has been > allocated (and is going to be initialized and used), not when it is > freed. To depend on initialization that happens elsewhere (when the > whole memory array is allocated and when individual data structures > are being freed) feels fragile. This dependency is not even documented > (with a comment), at least not here when the data structure is cleared > and freed. Someone might remove that memset (because it feels > redundant) and then break an assumption made by code elsewhere. > > > > > > > -----Original Message----- > > From: Ola Liljedahl [mailto:ola.liljedahl@linaro.org] > > Sent: Thursday, February 26, 2015 12:49 PM > > To: Anders Roxell; Taras Kondratiuk; Robbie King (robking) > > Cc: LNG ODP Mailman List > > Subject: Re: [lng-odp] [PATCH] linux-generic: odp_crypto: use intptr_t > to find the pointer > > > > On 26 February 2015 at 14:18, Anders Roxell <anders.roxell@linaro.org> > wrote: > >> commit 4a4804a breaks on ARM 32-bit cross compile. > >> odp_crypto.c: In function 'odp_crypto_session_destroy': > >> odp_crypto.c:364:12: error: cast to pointer from integer of different > >> size [-Werror=int-to-pointer-cast] > >> generic = (odp_crypto_generic_session_t *)session; > >> > >> Signed-off-by: Anders Roxell <anders.roxell@linaro.org> > >> --- > >> platform/linux-generic/odp_crypto.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/platform/linux-generic/odp_crypto.c > b/platform/linux-generic/odp_crypto.c > >> index 0c38263..2f13e2f 100644 > >> --- a/platform/linux-generic/odp_crypto.c > >> +++ b/platform/linux-generic/odp_crypto.c > >> @@ -361,7 +361,7 @@ int odp_crypto_session_destroy(odp_crypto_session_t > session) > >> { > >> odp_crypto_generic_session_t *generic; > >> > >> - generic = (odp_crypto_generic_session_t *)session; > >> + generic = (odp_crypto_generic_session_t *)(intptr_t)session; > >> memset(generic, 0, sizeof(*generic)); > > Why are we clearing the session structure before the free (as I assume > > it is) below? > > Shouldn't wipe on free be a generic malloc/free (conditional debug) > > feature and not a mandatory part of some module that uses malloc? > > Is the crypto session somehow considered sensitive data (it doesn't > > contain any actual keys?) so we really want to wipe it as soon as it > > is no longer needed? > > > >> free_session(generic); > >> return 0; > >> -- > >> 2.1.4 > >> > >> > >> _______________________________________________ > >> lng-odp mailing list > >> lng-odp@lists.linaro.org > >> http://lists.linaro.org/mailman/listinfo/lng-odp > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 26 February 2015 at 20:20, Robbie King (robking) <robking@cisco.com> wrote: > You are correct, "free_session" puts the session control structure > being returned back onto a free list maintained by crypto, it does > not return the shared memory back to the system. > > I have found clear/poison at free useful for catching stale pointers > in applications, where the pointer frees the structure but then > continues to access it. If it has been wiped or poisoned, there is > a better chance of catching this versus it staying valid (from the > application point of view) and then suddenly it's in use again > with different but valid fields. Wipe/poison on free is useful for e.g. debugging but I don't think one should rely on this to have happened when (re-) allocating the buffer. In my previous life, wipe on free was a run-time option on the heap allocator. When the system survived testing, this could be disabled for getting some additional performance. There were other related mechanisms, e.g. buffer endmarks, that were enabled by default but could be disabled. Some customers actually chose to run their production systems with a lot if these runtime checks enabled. In my opinion, clear of buffers should be the responsibility of the code that actually allocates and initializes the buffer. -- Ola > > > -----Original Message----- > From: Ola Liljedahl [mailto:ola.liljedahl@linaro.org] > Sent: Thursday, February 26, 2015 2:14 PM > To: Robbie King (robking) > Cc: Anders Roxell; Taras Kondratiuk; LNG ODP Mailman List > Subject: Re: [lng-odp] [PATCH] linux-generic: odp_crypto: use intptr_t to find the pointer > > On 26 February 2015 at 18:52, Robbie King (robking) <robking@cisco.com> wrote: >> When the memory is first allocated (at global_init), the entire memory >> array is cleared. I memset it on free such that initial state is the >> same at allocation time (whether it is the first allocation of the structure >> or a subsequent). Alternatively it can be done at allocation, but I think >> it should be done at some point before being (re)used. > So free_session() doesn't give back the memory to the real allocator? > > To me it feels better to clear a data structure just after it has been > allocated (and is going to be initialized and used), not when it is > freed. To depend on initialization that happens elsewhere (when the > whole memory array is allocated and when individual data structures > are being freed) feels fragile. This dependency is not even documented > (with a comment), at least not here when the data structure is cleared > and freed. Someone might remove that memset (because it feels > redundant) and then break an assumption made by code elsewhere. > > > >> >> -----Original Message----- >> From: Ola Liljedahl [mailto:ola.liljedahl@linaro.org] >> Sent: Thursday, February 26, 2015 12:49 PM >> To: Anders Roxell; Taras Kondratiuk; Robbie King (robking) >> Cc: LNG ODP Mailman List >> Subject: Re: [lng-odp] [PATCH] linux-generic: odp_crypto: use intptr_t to find the pointer >> >> On 26 February 2015 at 14:18, Anders Roxell <anders.roxell@linaro.org> wrote: >>> commit 4a4804a breaks on ARM 32-bit cross compile. >>> odp_crypto.c: In function 'odp_crypto_session_destroy': >>> odp_crypto.c:364:12: error: cast to pointer from integer of different >>> size [-Werror=int-to-pointer-cast] >>> generic = (odp_crypto_generic_session_t *)session; >>> >>> Signed-off-by: Anders Roxell <anders.roxell@linaro.org> >>> --- >>> platform/linux-generic/odp_crypto.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/platform/linux-generic/odp_crypto.c b/platform/linux-generic/odp_crypto.c >>> index 0c38263..2f13e2f 100644 >>> --- a/platform/linux-generic/odp_crypto.c >>> +++ b/platform/linux-generic/odp_crypto.c >>> @@ -361,7 +361,7 @@ int odp_crypto_session_destroy(odp_crypto_session_t session) >>> { >>> odp_crypto_generic_session_t *generic; >>> >>> - generic = (odp_crypto_generic_session_t *)session; >>> + generic = (odp_crypto_generic_session_t *)(intptr_t)session; >>> memset(generic, 0, sizeof(*generic)); >> Why are we clearing the session structure before the free (as I assume >> it is) below? >> Shouldn't wipe on free be a generic malloc/free (conditional debug) >> feature and not a mandatory part of some module that uses malloc? >> Is the crypto session somehow considered sensitive data (it doesn't >> contain any actual keys?) so we really want to wipe it as soon as it >> is no longer needed? >> >>> free_session(generic); >>> return 0; >>> -- >>> 2.1.4 >>> >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org >>> http://lists.linaro.org/mailman/listinfo/lng-odp
diff --git a/platform/linux-generic/odp_crypto.c b/platform/linux-generic/odp_crypto.c index 0c38263..2f13e2f 100644 --- a/platform/linux-generic/odp_crypto.c +++ b/platform/linux-generic/odp_crypto.c @@ -361,7 +361,7 @@ int odp_crypto_session_destroy(odp_crypto_session_t session) { odp_crypto_generic_session_t *generic; - generic = (odp_crypto_generic_session_t *)session; + generic = (odp_crypto_generic_session_t *)(intptr_t)session; memset(generic, 0, sizeof(*generic)); free_session(generic); return 0;
commit 4a4804a breaks on ARM 32-bit cross compile. odp_crypto.c: In function 'odp_crypto_session_destroy': odp_crypto.c:364:12: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast] generic = (odp_crypto_generic_session_t *)session; Signed-off-by: Anders Roxell <anders.roxell@linaro.org> --- platform/linux-generic/odp_crypto.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)