| Message ID | 20171128154236.19192-4-antoine.tenart@free-electrons.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
Hi Antoine, On 28.11.2017 16:42, Antoine Tenart wrote: > The patch fixes the ahash support by only updating the result buffer > when provided. Otherwise the driver could crash with NULL pointer > exceptions, because the ahash caller isn't required to supply a result > buffer on all calls. Can you point to bug crush report ? > Fixes: 1b44c5a60c13 ("crypto: inside-secure - add SafeXcel EIP197 crypto engine driver") > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> > --- > drivers/crypto/inside-secure/safexcel_hash.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c > index 6135c9f5742c..424f4c5d4d25 100644 > --- a/drivers/crypto/inside-secure/safexcel_hash.c > +++ b/drivers/crypto/inside-secure/safexcel_hash.c > @@ -150,7 +150,12 @@ static int safexcel_handle_req_result(struct safexcel_crypto_priv *priv, int rin > > if (sreq->finish) > result_sz = crypto_ahash_digestsize(ahash); > - memcpy(sreq->state, areq->result, result_sz); > + > + /* The called isn't required to supply a result buffer. Updated it only > + * when provided. > + */ > + if (areq->result) > + memcpy(sreq->state, areq->result, result_sz); > > dma_unmap_sg(priv->dev, areq->src, > sg_nents_for_len(areq->src, areq->nbytes), DMA_TO_DEVICE); > can the driver get request for final/finup/digest with null req->result ? If yes (?), such checks can be done before any hardware processing, saving time, for example: int hash_final(struct ahash_request *areq) { if (!areq->result) return -EINVAL; /* normal processing here */ }
Hi Kamil, On Thu, Nov 30, 2017 at 10:19:26AM +0100, Kamil Konieczny wrote: > On 28.11.2017 16:42, Antoine Tenart wrote: > > The patch fixes the ahash support by only updating the result buffer > > when provided. Otherwise the driver could crash with NULL pointer > > exceptions, because the ahash caller isn't required to supply a result > > buffer on all calls. > > Can you point to bug crush report ? Do you want the crash dump? (It'll only be a "normal" NULL pointer dereference). > > Fixes: 1b44c5a60c13 ("crypto: inside-secure - add SafeXcel EIP197 crypto engine driver") > > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> > > --- > > drivers/crypto/inside-secure/safexcel_hash.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c > > index 6135c9f5742c..424f4c5d4d25 100644 > > --- a/drivers/crypto/inside-secure/safexcel_hash.c > > +++ b/drivers/crypto/inside-secure/safexcel_hash.c > > @@ -150,7 +150,12 @@ static int safexcel_handle_req_result(struct safexcel_crypto_priv *priv, int rin > > > > if (sreq->finish) > > result_sz = crypto_ahash_digestsize(ahash); > > - memcpy(sreq->state, areq->result, result_sz); > > + > > + /* The called isn't required to supply a result buffer. Updated it only > > + * when provided. > > + */ > > + if (areq->result) > > + memcpy(sreq->state, areq->result, result_sz); > > > > dma_unmap_sg(priv->dev, areq->src, > > sg_nents_for_len(areq->src, areq->nbytes), DMA_TO_DEVICE); > > > > can the driver get request for final/finup/digest with null req->result ? I don't think that can happen. But having an update called without req->result provided is a valid call (though it's not well documented). Thanks, Antoine
Hi Antoine, On 30.11.2017 10:29, Antoine Tenart wrote: > Hi Kamil, > > On Thu, Nov 30, 2017 at 10:19:26AM +0100, Kamil Konieczny wrote: >> On 28.11.2017 16:42, Antoine Tenart wrote: >>> The patch fixes the ahash support by only updating the result buffer >>> when provided. Otherwise the driver could crash with NULL pointer >>> exceptions, because the ahash caller isn't required to supply a result >>> buffer on all calls. >> >> Can you point to bug crush report ? > > Do you want the crash dump? (It'll only be a "normal" NULL pointer > dereference). Ah I see, in this case not. >>> Fixes: 1b44c5a60c13 ("crypto: inside-secure - add SafeXcel EIP197 crypto engine driver") >>> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> >>> --- >>> drivers/crypto/inside-secure/safexcel_hash.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c >>> index 6135c9f5742c..424f4c5d4d25 100644 >>> --- a/drivers/crypto/inside-secure/safexcel_hash.c >>> +++ b/drivers/crypto/inside-secure/safexcel_hash.c >>> @@ -150,7 +150,12 @@ static int safexcel_handle_req_result(struct safexcel_crypto_priv *priv, int rin >>> >>> if (sreq->finish) >>> result_sz = crypto_ahash_digestsize(ahash); >>> - memcpy(sreq->state, areq->result, result_sz); >> [...] >> can the driver get request for final/finup/digest with null req->result ? > > I don't think that can happen. But having an update called without > req->result provided is a valid call (though it's not well documented). so maybe: if (sreq->finish) { result_sz = crypto_ahash_digestsize(ahash); memcpy(sreq->state, areq->result, result_sz); }
On Thu, Nov 30, 2017 at 12:52:42PM +0100, Kamil Konieczny wrote: > On 30.11.2017 10:29, Antoine Tenart wrote: > > On Thu, Nov 30, 2017 at 10:19:26AM +0100, Kamil Konieczny wrote: > >> can the driver get request for final/finup/digest with null req->result ? > > > > I don't think that can happen. But having an update called without > > req->result provided is a valid call (though it's not well documented). > > so maybe: > > if (sreq->finish) { > result_sz = crypto_ahash_digestsize(ahash); > memcpy(sreq->state, areq->result, result_sz); > } No, if we do this we'll lose the ability to export the current state. Thanks, Antoine
On 30.11.2017 13:41, Antoine Tenart wrote: > On Thu, Nov 30, 2017 at 12:52:42PM +0100, Kamil Konieczny wrote: >> On 30.11.2017 10:29, Antoine Tenart wrote: >>> On Thu, Nov 30, 2017 at 10:19:26AM +0100, Kamil Konieczny wrote: >>>> can the driver get request for final/finup/digest with null req->result ? >>> >>> I don't think that can happen. But having an update called without >>> req->result provided is a valid call (though it's not well documented). Yes, this field may be unset until finup/final. One more to watch out is that in final, you can have areq->nsize != 0 so you should not use areq->nsize nor areq->src >> so maybe: >> >> if (sreq->finish) { >> result_sz = crypto_ahash_digestsize(ahash); >> memcpy(sreq->state, areq->result, result_sz); >> } > > No, if we do this we'll lose the ability to export the current state. So maybe save it into request context: result_sz = crypto_ahash_digestsize(ahash); ctx = ahash_request_ctx(areq); if (sreq->finish) memcpy(sreq->state, areq->result, result_sz); else memcpy(sreq->state, ctx->state, result_sz);
Hi Kamil, On Thu, Nov 30, 2017 at 03:10:28PM +0100, Kamil Konieczny wrote: > On 30.11.2017 13:41, Antoine Tenart wrote: > > > > No, if we do this we'll lose the ability to export the current state. > > So maybe save it into request context: > > result_sz = crypto_ahash_digestsize(ahash); > ctx = ahash_request_ctx(areq); > > if (sreq->finish) > memcpy(sreq->state, areq->result, result_sz); > else > memcpy(sreq->state, ctx->state, result_sz); Storing the digest into a driver own buffer could improve the export ability in some *very rare* cases. If so I would suggest not to deal with the kind of test you proposed, but to have your own buffer used each time. Anyway, this has nothing to do with the fix I'm proposing here, as it would change the driver's logic, and would solve a complete different (rare) issue. The proposal here is to have a simple fix (which is similar to what can be found in some other drivers), that can easily be backported to avoid NULL pointer dereferences in older versions of the kernel. Thanks, Antoine
On Thu, Nov 30, 2017 at 10:19:26AM +0100, Kamil Konieczny wrote: > > can the driver get request for final/finup/digest with null req->result ? > If yes (?), such checks can be done before any hardware processing, saving time, > for example: This should not be possible through any user-space facing API. If a kernel API user did this then they're just shooting themselves in the foot. So unless there is a valida call path that leads to this then I would say that there is nothing to fix. Thanks,
Hi Herbert, On Fri, Dec 01, 2017 at 11:31:09AM +1100, Herbert Xu wrote: > On Thu, Nov 30, 2017 at 10:19:26AM +0100, Kamil Konieczny wrote: > > > > can the driver get request for final/finup/digest with null req->result ? > > If yes (?), such checks can be done before any hardware processing, saving time, > > for example: > > This should not be possible through any user-space facing API. > > If a kernel API user did this then they're just shooting themselves > in the foot. > > So unless there is a valida call path that leads to this then I > would say that there is nothing to fix. I agree this should not be the case. But: - Other drivers are doing this check (grep "if (!req->result)" or "if (req->result)" to see some of them). - I see at least one commit fixing the exact same issue I'm facing here, 393897c5156a415533ff85aa381458840417b032: crypto: ccp - Check for caller result area before using it For a hash operation, the caller doesn't have to supply a result area on every call so don't use it / update it if it hasn't been supplied. I'm not entirely sure what was the code path that leads to this, I'll reproduce the issue and try to understand what is going on (I clearly recall having this crash though). The crypto API does not enforce this somehow, and this should probably be fixed. That might break some users. But it was seen as a valid use for some, so we should probably fix this in previous versions of the driver anyway. Thanks! Antoine
Hi All, On 01.12.2017 09:11, Antoine Tenart wrote: > Hi Herbert, > > On Fri, Dec 01, 2017 at 11:31:09AM +1100, Herbert Xu wrote: >> On Thu, Nov 30, 2017 at 10:19:26AM +0100, Kamil Konieczny wrote: >>> >>> can the driver get request for final/finup/digest with null req->result ? >>> If yes (?), such checks can be done before any hardware processing, saving time, >>> for example: >> >> This should not be possible through any user-space facing API. >> >> If a kernel API user did this then they're just shooting themselves >> in the foot. >> >> So unless there is a valida call path that leads to this then I >> would say that there is nothing to fix. > > I agree this should not be the case. > > But: > - Other drivers are doing this check (grep "if (!req->result)" or > "if (req->result)" to see some of them). > - I see at least one commit fixing the exact same issue I'm facing here, > 393897c5156a415533ff85aa381458840417b032: > > crypto: ccp - Check for caller result area before using it > > For a hash operation, the caller doesn't have to supply a result > area on every call so don't use it / update it if it hasn't > been supplied. Herbert, is it possible for every init/update that areq->result can be NULL, and only for final/update/digit user set it to actual memory ? testmgr.c can check if hash update writes into areq->result and if yes, then test fails ? As I understand this, when crypto api user allocates ahash_request, crypto allocates memory for itself _plus_ for driver's context. This allocated ahash_request is "handle" for all subsequent updates/export/import, and for last final/finup, so I do not need to copy hash state into areq->result, but keep it whole time in context, in your code in sreq: struct safexcel_ahash_req *sreq = ahash_request_ctx(areq); so here sreq is async hash request context. Do you set last_req true for digest/finup/final ? If yes, then you need to copy result only when it is true, if (sreq->last_req) { result_sz = crypto_ahash_digestsize(ahash); memcpy(sreq->state, areq->result, result_sz); } I do not read all your code though, so I can be wrong here. > I'm not entirely sure what was the code path that leads to this, I'll > reproduce the issue and try to understand what is going on (I clearly > recall having this crash though). > > The crypto API does not enforce this somehow, and this should probably > be fixed. That might break some users. But it was seen as a valid use > for some, so we should probably fix this in previous versions of the > driver anyway.
Hi Kamil, On Fri, Dec 01, 2017 at 11:18:30AM +0100, Kamil Konieczny wrote: > On 01.12.2017 09:11, Antoine Tenart wrote: > > - Other drivers are doing this check (grep "if (!req->result)" or > > "if (req->result)" to see some of them). > > - I see at least one commit fixing the exact same issue I'm facing here, > > 393897c5156a415533ff85aa381458840417b032: > > > > crypto: ccp - Check for caller result area before using it > > > > For a hash operation, the caller doesn't have to supply a result > > area on every call so don't use it / update it if it hasn't > > been supplied. > > Do you set last_req true for digest/finup/final ? If yes, > then you need to copy result only when it is true, > > if (sreq->last_req) { > result_sz = crypto_ahash_digestsize(ahash); > memcpy(sreq->state, areq->result, result_sz); > } Yes the last_req flag is set for the last request, so when digest/finup/final are called. But no we can't copy the result into the state based on this as an user might want to perform multiple updates, then export the context, to import it again before sending more updates. Antoine
On Fri, Dec 01, 2017 at 09:11:57AM +0100, Antoine Tenart wrote: > > I agree this should not be the case. > > But: > - Other drivers are doing this check (grep "if (!req->result)" or > "if (req->result)" to see some of them). > - I see at least one commit fixing the exact same issue I'm facing here, > 393897c5156a415533ff85aa381458840417b032: > > crypto: ccp - Check for caller result area before using it > > For a hash operation, the caller doesn't have to supply a result > area on every call so don't use it / update it if it hasn't > been supplied. > > I'm not entirely sure what was the code path that leads to this, I'll > reproduce the issue and try to understand what is going on (I clearly > recall having this crash though). That's different. In that case an unconditional copy is made regardless of whether the operation is final or update. That's why a check is required. If the operation is finup/final/digest then req->result must be set and you don't need to check it. Cheers,
On Fri, Dec 01, 2017 at 11:18:30AM +0100, Kamil Konieczny wrote: > > Herbert, is it possible for every init/update that areq->result can be NULL, > and only for final/update/digit user set it to actual memory ? > testmgr.c can check if hash update writes into areq->result and if yes, > then test fails ? Yes we should modify testmgr to check that. Thanks,
Hi Antoine, On 01.12.2017 11:24, Antoine Tenart wrote: > Hi Kamil, > > On Fri, Dec 01, 2017 at 11:18:30AM +0100, Kamil Konieczny wrote: >> On 01.12.2017 09:11, Antoine Tenart wrote: >>> - Other drivers are doing this check (grep "if (!req->result)" or >>> "if (req->result)" to see some of them). >>> - I see at least one commit fixing the exact same issue I'm facing here, >>> 393897c5156a415533ff85aa381458840417b032: >>> >>> crypto: ccp - Check for caller result area before using it >>> >>> For a hash operation, the caller doesn't have to supply a result >>> area on every call so don't use it / update it if it hasn't >>> been supplied. >> >> Do you set last_req true for digest/finup/final ? If yes, >> then you need to copy result only when it is true, >> >> if (sreq->last_req) { >> result_sz = crypto_ahash_digestsize(ahash); >> memcpy(sreq->state, areq->result, result_sz); >> } > > Yes the last_req flag is set for the last request, so when > digest/finup/final are called. But no we can't copy the result into the > state based on this as an user might want to perform multiple updates, > then export the context, to import it again before sending more updates. IMHO set them to false in hash update and init, set finish false and last_req true for hash final, and set both true for hash finup and digest. As Herbert tells in https://www.spinics.net/lists/linux-crypto/msg28658.html you should support scenario export + update/finup, so basically export is reading state but it do not halt your hash driver.
Hi Herbert, On Fri, Dec 01, 2017 at 09:35:52PM +1100, Herbert Xu wrote: > On Fri, Dec 01, 2017 at 09:11:57AM +0100, Antoine Tenart wrote: > > > > I agree this should not be the case. > > > > But: > > - Other drivers are doing this check (grep "if (!req->result)" or > > "if (req->result)" to see some of them). > > - I see at least one commit fixing the exact same issue I'm facing here, > > 393897c5156a415533ff85aa381458840417b032: > > > > crypto: ccp - Check for caller result area before using it > > > > For a hash operation, the caller doesn't have to supply a result > > area on every call so don't use it / update it if it hasn't > > been supplied. > > > > I'm not entirely sure what was the code path that leads to this, I'll > > reproduce the issue and try to understand what is going on (I clearly > > recall having this crash though). > > That's different. In that case an unconditional copy is made > regardless of whether the operation is final or update. That's > why a check is required. > > If the operation is finup/final/digest then req->result must be > set and you don't need to check it. Ah, I didn't understand your point then. Of course ->result should be allocated for finup/final/digest. The function where I fix this is called regardless of the operation that was performed, so it can be an update() as well. Thanks, Antoine
On Fri, Dec 01, 2017 at 11:43:18AM +0100, Kamil Konieczny wrote: > On 01.12.2017 11:24, Antoine Tenart wrote: > > > > Yes the last_req flag is set for the last request, so when > > digest/finup/final are called. But no we can't copy the result into the > > state based on this as an user might want to perform multiple updates, > > then export the context, to import it again before sending more updates. > > IMHO set them to false in hash update and init, set finish false and last_req true > for hash final, and set both true for hash finup and digest. > > As Herbert tells in https://www.spinics.net/lists/linux-crypto/msg28658.html > you should support scenario export + update/finup, so basically export is reading > state but it do not halt your hash driver. Except if you import another state in the meantime. Antoine
Hi Herbert, On 01.12.2017 11:36, Herbert Xu wrote: > On Fri, Dec 01, 2017 at 11:18:30AM +0100, Kamil Konieczny wrote: >> >> Herbert, is it possible for every init/update that areq->result can be NULL, >> and only for final/update/digit user set it to actual memory ? >> testmgr.c can check if hash update writes into areq->result and if yes, >> then test fails ? > > Yes we should modify testmgr to check that. I can write patch for it. Should it WARN_ON or treat it as error ?
diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c index 6135c9f5742c..424f4c5d4d25 100644 --- a/drivers/crypto/inside-secure/safexcel_hash.c +++ b/drivers/crypto/inside-secure/safexcel_hash.c @@ -150,7 +150,12 @@ static int safexcel_handle_req_result(struct safexcel_crypto_priv *priv, int rin if (sreq->finish) result_sz = crypto_ahash_digestsize(ahash); - memcpy(sreq->state, areq->result, result_sz); + + /* The called isn't required to supply a result buffer. Updated it only + * when provided. + */ + if (areq->result) + memcpy(sreq->state, areq->result, result_sz); dma_unmap_sg(priv->dev, areq->src, sg_nents_for_len(areq->src, areq->nbytes), DMA_TO_DEVICE);
The patch fixes the ahash support by only updating the result buffer when provided. Otherwise the driver could crash with NULL pointer exceptions, because the ahash caller isn't required to supply a result buffer on all calls. Fixes: 1b44c5a60c13 ("crypto: inside-secure - add SafeXcel EIP197 crypto engine driver") Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> --- drivers/crypto/inside-secure/safexcel_hash.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)