| Message ID | 20200511111913.26541-4-t-kristo@ti.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: omap: sha/aes fixes | expand |
On Mon, May 11, 2020 at 02:19:09PM +0300, Tero Kristo wrote: > In case buffers are copied from userspace, directly accessing the page > will most likely fail because it hasn't been mapped into the kernel > memory space. Fix the issue by forcing a kmap / kunmap within the > cleanup functionality. > > Signed-off-by: Tero Kristo <t-kristo@ti.com> > --- > drivers/crypto/omap-crypto.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/crypto/omap-crypto.c b/drivers/crypto/omap-crypto.c > index cc88b7362bc2..31bdb1d76d11 100644 > --- a/drivers/crypto/omap-crypto.c > +++ b/drivers/crypto/omap-crypto.c > @@ -178,11 +178,16 @@ static void omap_crypto_copy_data(struct scatterlist *src, > amt = min(src->length - srco, dst->length - dsto); > amt = min(len, amt); > > - srcb = sg_virt(src) + srco; > - dstb = sg_virt(dst) + dsto; > + srcb = kmap_atomic(sg_page(src)) + srco + src->offset; > + dstb = kmap_atomic(sg_page(dst)) + dsto + dst->offset; > > memcpy(dstb, srcb, amt); > > + flush_dcache_page(sg_page(dst)); You need to check !PageSlab as it's illegal to call it on a kernel page. Also you should be using flush_kernel_dcache_page. scatterwalk uses flush_dcache_page only because when it was created the other function didn't exist. Would it be possible to use the sg_miter interface to do the copy? In fact your function could possibly be added to lib/scatterlist.c as it seems to be quite generic. Thanks,
On 22/05/2020 16:12, Herbert Xu wrote: > On Mon, May 11, 2020 at 02:19:09PM +0300, Tero Kristo wrote: >> In case buffers are copied from userspace, directly accessing the page >> will most likely fail because it hasn't been mapped into the kernel >> memory space. Fix the issue by forcing a kmap / kunmap within the >> cleanup functionality. >> >> Signed-off-by: Tero Kristo <t-kristo@ti.com> >> --- >> drivers/crypto/omap-crypto.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/crypto/omap-crypto.c b/drivers/crypto/omap-crypto.c >> index cc88b7362bc2..31bdb1d76d11 100644 >> --- a/drivers/crypto/omap-crypto.c >> +++ b/drivers/crypto/omap-crypto.c >> @@ -178,11 +178,16 @@ static void omap_crypto_copy_data(struct scatterlist *src, >> amt = min(src->length - srco, dst->length - dsto); >> amt = min(len, amt); >> >> - srcb = sg_virt(src) + srco; >> - dstb = sg_virt(dst) + dsto; >> + srcb = kmap_atomic(sg_page(src)) + srco + src->offset; >> + dstb = kmap_atomic(sg_page(dst)) + dsto + dst->offset; >> >> memcpy(dstb, srcb, amt); >> >> + flush_dcache_page(sg_page(dst)); > > You need to check !PageSlab as it's illegal to call it on a kernel > page. Also you should be using flush_kernel_dcache_page. scatterwalk > uses flush_dcache_page only because when it was created the other > function didn't exist. Ok will fix that. > Would it be possible to use the sg_miter interface to do the copy? > In fact your function could possibly be added to lib/scatterlist.c > as it seems to be quite generic. I think it would make sense to use that, however as I am just fixing an existing bug here, would it be ok if I just fix your above comment and post v3? I can convert this later to sg_miter and take a shot at moving this to lib/scatterlist.c as that seems slightly bigger effort and I would not want to block this whole series because of that... -Tero -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Tue, May 26, 2020 at 03:27:38PM +0300, Tero Kristo wrote: > > I think it would make sense to use that, however as I am just fixing an > existing bug here, would it be ok if I just fix your above comment and post > v3? I can convert this later to sg_miter and take a shot at moving this to > lib/scatterlist.c as that seems slightly bigger effort and I would not want > to block this whole series because of that... Of course. A minimal fix would be just fine. Thanks,
On 26/05/2020 15:35, Herbert Xu wrote: > On Tue, May 26, 2020 at 03:27:38PM +0300, Tero Kristo wrote: >> >> I think it would make sense to use that, however as I am just fixing an >> existing bug here, would it be ok if I just fix your above comment and post >> v3? I can convert this later to sg_miter and take a shot at moving this to >> lib/scatterlist.c as that seems slightly bigger effort and I would not want >> to block this whole series because of that... > > Of course. A minimal fix would be just fine. Ok thanks, will post fixed version hopefully already today, just running some tests on it now. Btw, any word on the TI sa2ul series I posted a while back? I see it has been marked as deferred in patchwork but I am not quite sure what that means... deferred until what? I have also been thinking of creating a drivers/crypto/ti subdir at some point, as there are quite a few files for TI accelerators already. -Tero -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On Tue, May 26, 2020 at 03:57:10PM +0300, Tero Kristo wrote: > > Btw, any word on the TI sa2ul series I posted a while back? I see it has > been marked as deferred in patchwork but I am not quite sure what that > means... deferred until what? I have also been thinking of creating a > drivers/crypto/ti subdir at some point, as there are quite a few files for > TI accelerators already. IIRC it was missing an ack from Rob Herring, unless you want me to only apply the bits under drivers/crypto? Cheers,
On 26/05/2020 16:07, Herbert Xu wrote: > On Tue, May 26, 2020 at 03:57:10PM +0300, Tero Kristo wrote: >> >> Btw, any word on the TI sa2ul series I posted a while back? I see it has >> been marked as deferred in patchwork but I am not quite sure what that >> means... deferred until what? I have also been thinking of creating a >> drivers/crypto/ti subdir at some point, as there are quite a few files for >> TI accelerators already. > > IIRC it was missing an ack from Rob Herring, unless you want me to > only apply the bits under drivers/crypto? Right, its missing ack from Rob still so need to wait for that. Was wondering if you had any comments on the actual patches themselves. -Tero -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
diff --git a/drivers/crypto/omap-crypto.c b/drivers/crypto/omap-crypto.c index cc88b7362bc2..31bdb1d76d11 100644 --- a/drivers/crypto/omap-crypto.c +++ b/drivers/crypto/omap-crypto.c @@ -178,11 +178,16 @@ static void omap_crypto_copy_data(struct scatterlist *src, amt = min(src->length - srco, dst->length - dsto); amt = min(len, amt); - srcb = sg_virt(src) + srco; - dstb = sg_virt(dst) + dsto; + srcb = kmap_atomic(sg_page(src)) + srco + src->offset; + dstb = kmap_atomic(sg_page(dst)) + dsto + dst->offset; memcpy(dstb, srcb, amt); + flush_dcache_page(sg_page(dst)); + + kunmap_atomic(srcb); + kunmap_atomic(dstb); + srco += amt; dsto += amt; len -= amt;
In case buffers are copied from userspace, directly accessing the page will most likely fail because it hasn't been mapped into the kernel memory space. Fix the issue by forcing a kmap / kunmap within the cleanup functionality. Signed-off-by: Tero Kristo <t-kristo@ti.com> --- drivers/crypto/omap-crypto.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)