Re: [Part2 PATCH v6 13/38] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

2017-10-27 Thread Borislav Petkov 
On Fri, Oct 27, 2017 at 05:59:23PM -0500, Brijesh Singh wrote:
> Yes it is typo. PEK_GEN wants FW to be in INIT state hence someone need
> to transition from UNINIT -> INIT.

Which, once you've done it once on driver init, is there.

> That's what I am doing except FACTORY_RESET.

Well, not really. Lemme pick a command at random...

PEK_CSR. For that, you do INIT -> PEK_CSR -> SHUTDOWN.

Doc says, platform needs to be in INIT or WORKING state. But nothing
says you should shut it down. Spec says, SHUTDOWN transitions platform
to UNINIT state. So when the next command comes in which needs the
platform to be in INIT state, you go and INIT it again. For no reason
*WHATSOEVER*!

I know, you're gonna say, but what if the next command needs a different
state than INIT. Well, *then* you transition it, in the command
function. When that function executes. But not before that and not in
preparation that *maybe* the next command will be it.

Now, if you did:

INIT once during driver init

PEK_CSR

(platform remains in INIT state)

<--- the next command here can execute directly if it is allowed in INIT
state.

Instead, the platform has been shutdown and you init it again. Do you
see now what I mean?

IOW, once you init the PSP master, you should keep it in the INIT state
- or the state in which most commands expect it to be and thus save
yourself all that unnecessary toggling. If a command needs it to be in a
different state, only *then* you transition it.

Instead, what you have now is that you call INIT and SHUTDOWN
around SEV_PEK_GEN, SEV_PDH_GEN, SEV_PEK_CSR, SEV_PEK_CERT_IMPORT,
SEV_PDH_CERT_EXPORT and for all those, the platform must be in INIT
(for some in WORKING state) but for all in INIT state and "The platform
remains be in the same state after completion." So the whole SHUTDOWN ->
INIT wankery in-between is a pure waste of electrons.

>  I see that we can do a small optimization -- since we already know
> the FW state hence we can avoid issuing PSP command when we know for
> sure that command will fail because we are not in correct state.

As I said before, you should do that regardless by recording the current
state of the PSP in variable so that you can save yourself the status
querying.

> If command needs INIT state and FW is not in INIT state then its safe to
> transition from UNINIT -> INIT. But if command needs UNINIT state and FW
> is in INIT state then its not safe to transition -- in those case we
> simply return EBUSY and let the user retry the command.

Whatever - that doesn't contradict what I'm proposing.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [Part2 PATCH v6 13/38] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

2017-10-27 Thread Borislav Petkov 
On Fri, Oct 27, 2017 at 04:28:31PM -0500, Brijesh Singh wrote:
> This will fail because PEK_GEN require the platform in INIT state and
> nobody has done the state transition from INIT -> UINIT.

Huh, FW is in INIT state and PEK_GEN wants it to be in INIT state. Typo?

Aaanyway, I don't like this whole notion of prematurely and predictively
executing commands on the PSP if it is not needed. So how about
executing only those commands which put the FW in the required state and
then executing the actual command?

I.e., if a command needs to be executed in UINIT state, you put the PSP
in that state before executing that command. If the command needs to be
in INIT state, you put the PSP in INIT state first and so on...

For convenience, you could carry the current PSP state in some struct
psp_dev member or whatever and query it before running the respective
commands.

Hmmm?

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [Part2 PATCH v6 13/38] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

2017-10-27 Thread Brijesh Singh 


On 10/27/17 3:27 PM, Borislav Petkov wrote:
> On Fri, Oct 27, 2017 at 03:25:24PM -0500, Brijesh Singh wrote:
>> Yep, we are doing state transition only when we really need to. At least
>> so far I have tried to avoid making any unnecessary state transitions.
> So change all those which do INIT -> CMD -> SHUTDOWN to do only the
> command as the INIT state is the default state, AFAIU it.
>
I don't that will work. It may be possible that I am not able to follow
what exactly you have in mind. Please let me clarify it, on boot the
firmware is in UINIT state.

Firmware maintain three states: UINIT, INIT and WORKING. Few commands
can be executed in UINIT, several command can be executed in INIT, some
command can be executed in WORKING and few commands can be executed in
any states (see SEV spec for platform state). We do not have ioctls to
issue the INIT and SHUTDOWN command.

As I have explained in previous messages, the SHUTDOWN unconditionally
destory's the FW context hence we have refrained from providing the
access for this command to userspace.

My approach is, when userspace issues a command we check if command
requires INIT state, if so then we do INIT -> CMD -> SHUTDOWN. If
command can be executed in any state then we issue the command . If
command need to be executed in UINIT state then we *do not* do
SHUTDOWN->CMD, instead we issue the cmd and PSP will fail with error
code and caller can check the error code to determine why command failed.

If I go with your recommendation then I am not able to see who will
transition the platform from UINIT -> INIT so that command can run?

Lets try with very simple example:

# modprobe ccp

/* FW is in INIT state */

# userspace runs

ioctl(fd, SEV_USER_PEK_GEN, )

This will fail because PEK_GEN require the platform in INIT state and
nobody has done the state transition from INIT -> UINIT.


-Brijesh

Re: [Part2 PATCH v6 13/38] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

2017-10-27 Thread Borislav Petkov 
On Fri, Oct 27, 2017 at 03:25:24PM -0500, Brijesh Singh wrote:
> Yep, we are doing state transition only when we really need to. At least
> so far I have tried to avoid making any unnecessary state transitions.

So change all those which do INIT -> CMD -> SHUTDOWN to do only the
command as the INIT state is the default state, AFAIU it.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [Part2 PATCH v6 13/38] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

2017-10-27 Thread Brijesh Singh 


On 10/27/17 3:15 PM, Borislav Petkov wrote:
> On Fri, Oct 27, 2017 at 06:28:38AM -0500, Brijesh Singh wrote:
>> ... User can retry the command sometime later when nobody else is
>> using the PSP.
> That still doesn't prevent you from doing two things:
>
> * make that fw_init_count a proper kref instead of your homegrown thing

OK, I can use kref in next patch.

>
> * do not preemptively execute commands on the PSP if you can't possibly
> know what the next command is going to be - instead, just put it in the
> required state only when you really have to. I.e., don't do all that
> unnecessary INIT -> CMD -> SHUTDOWN game for no reason.

Yep, we are doing state transition only when we really need to. At least
so far I have tried to avoid making any unnecessary state transitions.

> Thx.
>

Re: [Part2 PATCH v6 13/38] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

2017-10-27 Thread Borislav Petkov 
On Fri, Oct 27, 2017 at 06:28:38AM -0500, Brijesh Singh wrote:
> ... User can retry the command sometime later when nobody else is
> using the PSP.

That still doesn't prevent you from doing two things:

* make that fw_init_count a proper kref instead of your homegrown thing

* do not preemptively execute commands on the PSP if you can't possibly
know what the next command is going to be - instead, just put it in the
required state only when you really have to. I.e., don't do all that
unnecessary INIT -> CMD -> SHUTDOWN game for no reason.

Thx.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

[PATCH net-next 2/2] chcr: Add support for Inline IPSec

2017-10-27 Thread Atul Gupta 
register xfrmdev_ops callbacks, Send IPsec tunneled data
to HW for inline processing.
The driver use hardware crypto accelerator to encrypt and
generate ICV for the transmitted packet in Inline mode.

Signed-off-by: Atul Gupta 
Signed-off-by: Harsh Jain 
Signed-off-by: Ganesh Goudar 
---
 drivers/crypto/chelsio/Makefile  |   2 +-
 drivers/crypto/chelsio/chcr_algo.c   |  45 ++-
 drivers/crypto/chelsio/chcr_algo.h   |  17 -
 drivers/crypto/chelsio/chcr_core.c   |   8 +
 drivers/crypto/chelsio/chcr_core.h   |  39 +++
 drivers/crypto/chelsio/chcr_crypto.h |  17 +-
 drivers/crypto/chelsio/chcr_ipsec.c  | 659 +++
 7 files changed, 740 insertions(+), 47 deletions(-)
 create mode 100644 drivers/crypto/chelsio/chcr_ipsec.c

diff --git a/drivers/crypto/chelsio/Makefile b/drivers/crypto/chelsio/Makefile
index bebdf06..9056420 100644
--- a/drivers/crypto/chelsio/Makefile
+++ b/drivers/crypto/chelsio/Makefile
@@ -1,4 +1,4 @@
 ccflags-y := -Idrivers/net/ethernet/chelsio/cxgb4
 
 obj-$(CONFIG_CRYPTO_DEV_CHELSIO) += chcr.o
-chcr-objs :=  chcr_core.o chcr_algo.o
+chcr-objs :=  chcr_core.o chcr_algo.o chcr_ipsec.o
diff --git a/drivers/crypto/chelsio/chcr_algo.c 
b/drivers/crypto/chelsio/chcr_algo.c
index 0e81607..060f747 100644
--- a/drivers/crypto/chelsio/chcr_algo.c
+++ b/drivers/crypto/chelsio/chcr_algo.c
@@ -70,6 +70,29 @@
 #include "chcr_algo.h"
 #include "chcr_crypto.h"
 
+unsigned int sgl_ent_len[] = {0, 0, 16, 24, 40,
+ 48, 64, 72, 88,
+ 96, 112, 120, 136,
+ 144, 160, 168, 184, 192};
+
+unsigned int dsgl_ent_len[] = {0, 32, 32, 48, 48, 64, 64, 80, 80,
+  112, 112, 128, 128, 144, 144, 160, 160,
+  192, 192, 208, 208, 224, 224, 240, 240,
+  272, 272, 288, 288, 304, 304, 320, 320};
+
+static u32 round_constant[11] = {
+   0x0100, 0x0200, 0x0400, 0x0800,
+   0x1000, 0x2000, 0x4000, 0x8000,
+   0x1B00, 0x3600, 0x6C00
+};
+
+static int is_newsg(struct scatterlist *sgl, unsigned int *newents);
+static struct scatterlist *alloc_new_sg(struct scatterlist *sgl,
+   unsigned int nents);
+static inline void free_new_sg(struct scatterlist *sgl);
+static int chcr_handle_cipher_resp(struct ablkcipher_request *req,
+  unsigned char *input, int err);
+
 static inline  struct chcr_aead_ctx *AEAD_CTX(struct chcr_context *ctx)
 {
return ctx->crypto_ctx->aeadctx;
@@ -105,18 +128,6 @@ static inline int is_ofld_imm(const struct sk_buff *skb)
return (skb->len <= CRYPTO_MAX_IMM_TX_PKT_LEN);
 }
 
-/*
- * sgl_len - calculates the size of an SGL of the given capacity
- * @n: the number of SGL entries
- * Calculates the number of flits needed for a scatter/gather list that
- * can hold the given number of entries.
- */
-static inline unsigned int sgl_len(unsigned int n)
-{
-   n--;
-   return (3 * n) / 2 + (n & 1) + 2;
-}
-
 static void chcr_verify_tag(struct aead_request *req, u8 *input, int *err)
 {
u8 temp[SHA512_DIGEST_SIZE];
@@ -432,7 +443,7 @@ static inline int map_writesg_phys_cpl(struct device *dev,
return 0;
 }
 
-static inline int get_aead_subtype(struct crypto_aead *aead)
+inline int get_aead_subtype(struct crypto_aead *aead)
 {
struct aead_alg *alg = crypto_aead_alg(aead);
struct chcr_alg_template *chcr_crypto_alg =
@@ -3134,10 +3145,10 @@ static int chcr_aead_decrypt(struct aead_request *req)
}
 }
 
-static int chcr_aead_op(struct aead_request *req,
- unsigned short op_type,
- int size,
- create_wr_t create_wr_fn)
+int chcr_aead_op(struct aead_request *req,
+unsigned short op_type,
+int size,
+create_wr_t create_wr_fn)
 {
struct crypto_aead *tfm = crypto_aead_reqtfm(req);
struct chcr_context *ctx = crypto_aead_ctx(tfm);
diff --git a/drivers/crypto/chelsio/chcr_algo.h 
b/drivers/crypto/chelsio/chcr_algo.h
index 583008d..a5f5b19 100644
--- a/drivers/crypto/chelsio/chcr_algo.h
+++ b/drivers/crypto/chelsio/chcr_algo.h
@@ -217,7 +217,6 @@
ULP_TX_SC_MORE_V((immdatalen) ? 0 : 1))
 
 #define MAX_NK 8
-#define CRYPTO_MAX_IMM_TX_PKT_LEN 256
 #define MAX_WR_SIZE512
 #define ROUND_16(bytes)((bytes) & 0xFFF0)
 #define MAX_DSGL_ENT   32
@@ -230,16 +229,6 @@
 #define SPACE_LEFT(len) \
((MAX_WR_SIZE - WR_MIN_LEN - (len)))
 
-unsigned int sgl_ent_len[] = {0, 0, 16, 24, 40,
-   48, 64, 72, 88,
-   96, 112, 120, 136,
-   144, 160, 168, 184,
-

[PATCH net-next 1/2] cxgb4: Add support for Inline IPSec Tx

2017-10-27 Thread Atul Gupta 
Added Tx routine for ULD
- define interface for ULD Tx.

Export routines used for Tx data
- Routines common for data transmit are used by cxgb4 and chcr
  drivers.
- EXPORT routines enable transmit from chcr driver.

Signed-off-by: Atul Gupta 
Signed-off-by: Ganesh Goudar 
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4.h |  23 +
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c |   2 +
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c|   2 +-
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_uld.c |   1 +
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_uld.h |   3 +
 drivers/net/ethernet/chelsio/cxgb4/sge.c   | 101 ++---
 6 files changed, 80 insertions(+), 52 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h 
b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
index 92a0b02..ee3832a 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
@@ -58,6 +58,13 @@
 extern struct list_head adapter_list;
 extern struct mutex uld_mutex;
 
+/* Suspend an Ethernet Tx queue with fewer available descriptors than this.
+ * This is the same as calc_tx_descs() for a TSO packet with
+ * nr_frags == MAX_SKB_FRAGS.
+ */
+#define ETHTXQ_STOP_THRES \
+   (1 + DIV_ROUND_UP((3 * MAX_SKB_FRAGS) / 2 + (MAX_SKB_FRAGS & 1), 8))
+
 enum {
MAX_NPORTS  = 4, /* max # of ports */
SERNUM_LEN  = 24,/* Serial # length */
@@ -555,6 +562,7 @@ enum { /* adapter flags */
 
 enum {
ULP_CRYPTO_LOOKASIDE = 1 << 0,
+   ULP_CRYPTO_IPSEC_INLINE = 1 << 1,
 };
 
 struct rx_sw_desc;
@@ -957,6 +965,11 @@ enum {
SCHED_CLASS_RATEMODE_ABS = 1,   /* Kb/s */
 };
 
+struct tx_sw_desc {/* SW state per Tx descriptor */
+   struct sk_buff *skb;
+   struct ulptx_sgl *sgl;
+};
+
 /* Support for "sched_queue" command to allow one or more NIC TX Queues
  * to be bound to a TX Scheduling Class.
  */
@@ -1662,4 +1675,14 @@ void t4_tp_mib_read(struct adapter *adap, u32 *buff, u32 
nregs,
 void free_tx_desc(struct adapter *adap, struct sge_txq *q,
  unsigned int n, bool unmap);
 void free_txq(struct adapter *adap, struct sge_txq *q);
+inline void cxgb4_reclaim_completed_tx(struct adapter *adap,
+  struct sge_txq *q, bool unmap);
+int cxgb4_map_skb(struct device *dev, const struct sk_buff *skb,
+ dma_addr_t *addr);
+void cxgb4_inline_tx_skb(const struct sk_buff *skb, const struct sge_txq *q,
+void *pos);
+void cxgb4_write_sgl(const struct sk_buff *skb, struct sge_txq *q,
+struct ulptx_sgl *sgl, u64 *end, unsigned int start,
+const dma_addr_t *addr);
+inline void cxgb4_ring_tx_db(struct adapter *adap, struct sge_txq *q, int n);
 #endif /* __CXGB4_H__ */
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c 
b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
index 917663b..cf47183 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
@@ -3096,6 +3096,8 @@ static int chcr_show(struct seq_file *seq, void *v)
   atomic_read(>chcr_stats.error));
seq_printf(seq, "Fallback: %10u \n",
   atomic_read(>chcr_stats.fallback));
+   seq_printf(seq, "IPSec PDU: %10u\n",
+  atomic_read(>chcr_stats.ipsec_cnt));
return 0;
 }
 
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c 
b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index e16078d..538a8a7 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -4079,7 +4079,7 @@ static int adap_init0(struct adapter *adap)
} else {
adap->vres.ncrypto_fc = val[0];
}
-   adap->params.crypto |= ULP_CRYPTO_LOOKASIDE;
+   adap->params.crypto = ntohs(caps_cmd.cryptocaps);
adap->num_uld += 1;
}
 #undef FW_PARAM_PFVF
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_uld.c 
b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_uld.c
index 71a315b..6b5fea4 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_uld.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_uld.c
@@ -637,6 +637,7 @@ static void uld_init(struct adapter *adap, struct 
cxgb4_lld_info *lld)
lld->nchan = adap->params.nports;
lld->nports = adap->params.nports;
lld->wr_cred = adap->params.ofldq_wr_cred;
+   lld->crypto = adap->params.crypto;
lld->iscsi_iolen = MAXRXDATA_G(t4_read_reg(adap, TP_PARA_REG2_A));
lld->iscsi_tagmask = t4_read_reg(adap, ULP_RX_ISCSI_TAGMASK_A);
lld->iscsi_pgsz_order = t4_read_reg(adap, ULP_RX_ISCSI_PSZ_A);
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_uld.h 
b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_uld.h
index

[PATCH] hw_random: core: Reset user selected rng by writing "" to rng_current

2017-10-27 Thread PrasannaKumar Muralidharan 
User is able to select a chosen rng by writing its name to rng_current
but there is no way to reset it without unbinding the rng. Let user
write "" to rng_current and delesect the chosen rng.

Signed-off-by: PrasannaKumar Muralidharan 
---
 drivers/char/hw_random/core.c | 51 +++
 1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 9701ac7..be03024 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -292,26 +292,48 @@ static struct miscdevice rng_miscdev = {
.groups = rng_dev_groups,
 };
 
+static int enable_best_rng(void)
+{
+   int ret = -ENODEV;
+
+   BUG_ON(!mutex_is_locked(_mutex));
+
+   /* rng_list is sorted by quality, use the best (=first) one */
+   if (!list_empty(_list)) {
+   struct hwrng *new_rng;
+
+   new_rng = list_entry(rng_list.next, struct hwrng, list);
+   ret = ((new_rng == current_rng) ? 0 : set_current_rng(new_rng));
+   if (!ret)
+   cur_rng_set_by_user = 0;
+   }
+
+   return ret;
+}
+
 static ssize_t hwrng_attr_current_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t len)
 {
-   int err;
+   int err = -ENODEV;
struct hwrng *rng;
 
err = mutex_lock_interruptible(_mutex);
if (err)
return -ERESTARTSYS;
-   err = -ENODEV;
-   list_for_each_entry(rng, _list, list) {
-   if (sysfs_streq(rng->name, buf)) {
-   err = 0;
-   cur_rng_set_by_user = 1;
-   if (rng != current_rng)
+
+   if (sysfs_streq(buf, "")) {
+   err = enable_best_rng();
+   } else {
+   list_for_each_entry(rng, _list, list) {
+   if (sysfs_streq(rng->name, buf)) {
+   cur_rng_set_by_user = 1;
err = set_current_rng(rng);
-   break;
+   break;
+   }
}
}
+
mutex_unlock(_mutex);
 
return err ? : len;
@@ -493,17 +515,8 @@ void hwrng_unregister(struct hwrng *rng)
mutex_lock(_mutex);
 
list_del(>list);
-   if (current_rng == rng) {
-   drop_current_rng();
-   cur_rng_set_by_user = 0;
-   /* rng_list is sorted by quality, use the best (=first) one */
-   if (!list_empty(_list)) {
-   struct hwrng *new_rng;
-
-   new_rng = list_entry(rng_list.next, struct hwrng, list);
-   set_current_rng(new_rng);
-   }
-   }
+   if (current_rng == rng)
+   enable_best_rng();
 
if (list_empty(_list)) {
mutex_unlock(_mutex);
-- 
2.10.0

[PATCH v2] staging: ccree: Fix indentation in ssi_buffer_mgr.c

2017-10-27 Thread Stephen Brennan 
In particular, fixes some over-indented if statement bodies as well as a
couple lines indented with spaces. checkpatch.pl now reports no warnings
on this file other than 80 character warnings.

Signed-off-by: Stephen Brennan 
---
V2 also fixes an overlooked indentation error that checkpatch didn't catch.

 drivers/staging/ccree/ssi_buffer_mgr.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c 
b/drivers/staging/ccree/ssi_buffer_mgr.c
index dca3ce84d043..1f8a225530a8 100644
--- a/drivers/staging/ccree/ssi_buffer_mgr.c
+++ b/drivers/staging/ccree/ssi_buffer_mgr.c
@@ -406,8 +406,8 @@ ssi_aead_handle_config_buf(struct device *dev,
sg_init_one(_ctx->ccm_adata_sg, config_data, AES_BLOCK_SIZE + 
areq_ctx->ccm_hdr_size);
if (unlikely(dma_map_sg(dev, _ctx->ccm_adata_sg, 1,
DMA_TO_DEVICE) != 1)) {
-   dev_err(dev, "dma_map_sg() config buffer failed\n");
-   return -ENOMEM;
+   dev_err(dev, "dma_map_sg() config buffer failed\n");
+   return -ENOMEM;
}
dev_dbg(dev, "Mapped curr_buff: dma_address=%pad page=%p addr=%pK 
offset=%u length=%u\n",
_dma_address(_ctx->ccm_adata_sg),
@@ -435,8 +435,8 @@ static inline int ssi_ahash_handle_curr_buf(struct device 
*dev,
sg_init_one(areq_ctx->buff_sg, curr_buff, curr_buff_cnt);
if (unlikely(dma_map_sg(dev, areq_ctx->buff_sg, 1,
DMA_TO_DEVICE) != 1)) {
-   dev_err(dev, "dma_map_sg() src buffer failed\n");
-   return -ENOMEM;
+   dev_err(dev, "dma_map_sg() src buffer failed\n");
+   return -ENOMEM;
}
dev_dbg(dev, "Mapped curr_buff: dma_address=%pad page=%p addr=%pK 
offset=%u length=%u\n",
_dma_address(areq_ctx->buff_sg), sg_page(areq_ctx->buff_sg),
@@ -1029,12 +1029,12 @@ static inline int ssi_buffer_mgr_prepare_aead_data_mlli(
 * verification is made by CPU compare in order to 
simplify
 * MAC verification upon request completion
 */
- u32 size_to_skip = req->assoclen;
+   u32 size_to_skip = req->assoclen;
 
- if (areq_ctx->is_gcm4543)
- size_to_skip += crypto_aead_ivsize(tfm);
+   if (areq_ctx->is_gcm4543)
+   size_to_skip += crypto_aead_ivsize(tfm);
 
- ssi_buffer_mgr_copy_scatterlist_portion(
+   ssi_buffer_mgr_copy_scatterlist_portion(
dev, areq_ctx->backup_mac, req->src,
size_to_skip + req->cryptlen - 
areq_ctx->req_authsize,
size_to_skip + req->cryptlen, SSI_SG_TO_BUF);
-- 
2.14.2

Re: [PATCH] staging: ccree: Fix indentation in ssi_buffer_mgr.c

2017-10-27 Thread Stephen Brennan 
On Fri, Oct 27, 2017 at 07:23:58AM -0700, Joe Perches wrote:
> On Fri, 2017-10-27 at 11:32 +0300, Dan Carpenter wrote:
> > But then ssi_buffer_mgr_copy_scatterlist_portion() is still not indented
> > correctly.

Yeah, I don't know how I missed it. I'll send a new version of the patch
out momentarily.

> The latest version of checkpatch should bleat something on the
>   ...scatterlist_portion(
> line

Strangely, my checkpatch doesn't complain about it, but I guess you can't
always rely on tools for this stuff. For reference, I'm currently on the
10/18 linux-next branch.

Re: [PATCH] staging: ccree: Fix indentation in ssi_buffer_mgr.c

2017-10-27 Thread Joe Perches 
On Fri, 2017-10-27 at 11:32 +0300, Dan Carpenter wrote:
> On Thu, Oct 26, 2017 at 06:53:42PM -0700, Stephen Brennan wrote:
> > In particular, fixes some over-indented if statement bodies as well as a
> > couple lines indented with spaces. checkpatch.pl now reports no warnings
> > on this file other than 80 character warnings.
[]
> > diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c 
> > b/drivers/staging/ccree/ssi_buffer_mgr.c
[]
> > @@ -1029,10 +1029,10 @@ static inline int 
> > ssi_buffer_mgr_prepare_aead_data_mlli(
> >  * verification is made by CPU compare in order to 
> > simplify
> >  * MAC verification upon request completion
> >  */
> > - u32 size_to_skip = req->assoclen;
> > +   u32 size_to_skip = req->assoclen;
> >  
> > - if (areq_ctx->is_gcm4543)
> > - size_to_skip += crypto_aead_ivsize(tfm);
> > +   if (areq_ctx->is_gcm4543)
> > +   size_to_skip += crypto_aead_ivsize(tfm);
> >  
> >   ssi_buffer_mgr_copy_scatterlist_portion(
> > dev, areq_ctx->backup_mac, req->src,
> 
> But then ssi_buffer_mgr_copy_scatterlist_portion() is still not indented
> correctly.

The latest version of checkpatch should bleat something on the
...scatterlist_portion(
line

Re: [PATCH 0/4] add CAAM DMA memcpy driver

2017-10-27 Thread Horia Geantă 
On 10/27/2017 2:36 PM, Koul, Vinod wrote:
>> On 10/26/2017 1:01 PM, Radu Alexe wrote:
>>> This patch-set introduces a new DMA memcpy driver based on the DMA
>>> capabilities of the CAAM crypto engine. Because of this dependency the
>>> included commits target various parts of the kernel tree.
>> I don't see the patches on any of the mail lists.
>> If they are moderated, I would expect this to be mentioned in the
>> MAINTAINERS file.
>>
>>>
>>> Patch 1.
>>> Since the CAAM DMA driver is a platform driver it is enabled by a new node
>>> in the device tree. This commit adds documentation for the device tree
>>> bindings.
>>>
>>> Patch 2.
>>> This patch adds the "caam-dma" node in the fsl-ls1012a.dtsi file.
>>>
>>> Patch 3.
>>> This commit adds various capabilities in the JR driver of the CAAM that is
>>> used by the CAAM DMA driver.
>>>
>>> Patch 4.
>>> Adds the CAAM DMA memcpy driver.
>>>
>>> Patch 1 and 3 should be merged by the crypto maintainers, patch 2 by
>>> devicetree maintainers and patch 4 by the DMA maintainers.
>> I would go with all the patches through the dmaengine tree.
> 
> Good call, but no one CCed dmaengine folks on this series!
> 
I see dmaeng...@vger.kernel.org Cc-ed on both cover letter and all four
patches.
It might be that the sender (Radu) is not subscribed and messages are
awaiting moderators' approval.

Horia

Re: Kernel panic when using ccm(aes) with the Atmel AES HW accelerator

2017-10-27 Thread Romain Izard 
2017-10-26 14:34 GMT+02:00 Tudor Ambarus :
> Hi, Romain,
>
> On 10/18/2017 04:32 PM, Romain Izard wrote:
>>
>> diff --git a/drivers/crypto/atmel-aes.c b/drivers/crypto/atmel-aes.c
>> index 29e20c37f3a6..f3eabe1f1490 100644
>> --- a/drivers/crypto/atmel-aes.c
>> +++ b/drivers/crypto/atmel-aes.c
>> @@ -80,6 +80,7 @@
>>   #define AES_FLAGS_BUSY BIT(3)
>>   #define AES_FLAGS_DUMP_REG BIT(4)
>>   #define AES_FLAGS_OWN_SHA  BIT(5)
>> +#define AES_FLAGS_CIPHERTAIL   BIT(6)
>
>
> not really needed, see below.
>
>>
>>   #define AES_FLAGS_PERSISTENT   (AES_FLAGS_INIT | AES_FLAGS_BUSY)
>>
>> @@ -156,6 +157,7 @@ struct atmel_aes_authenc_ctx {
>>
>>   struct atmel_aes_reqctx {
>>  unsigned long   mode;
>> +   u32 ciphertail[AES_BLOCK_SIZE / sizeof(u32)];
>
> How about renaming this variable to "lastc"? Ci, i=1..n is the usual
> notation for the ciphertext blocks.
>
OK


> The assumption that the last ciphertext block is of AES_BLOCK_SIZE size
> is correct, this driver is meant just for AES.
>
>>   };
>>
>>   #ifdef CONFIG_CRYPTO_DEV_ATMEL_AUTHENC
>> @@ -496,6 +498,11 @@ static void atmel_aes_authenc_complete(struct
>> atmel_aes_dev *dd, int err);
>>
>>   static inline int atmel_aes_complete(struct atmel_aes_dev *dd, int err)
>>   {
>> +   struct ablkcipher_request *req =
>> ablkcipher_request_cast(dd->areq);
>> +   struct crypto_ablkcipher *ablkcipher =
>> crypto_ablkcipher_reqtfm(req);
>> +   struct atmel_aes_reqctx *rctx = ablkcipher_request_ctx(req);
>> +   int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
>> +
>>   #ifdef CONFIG_CRYPTO_DEV_ATMEL_AUTHENC
>>  atmel_aes_authenc_complete(dd, err);
>>   #endif
>> @@ -503,6 +510,17 @@ static inline int atmel_aes_complete(struct
>> atmel_aes_dev *dd, int err)
>>  clk_disable(dd->iclk);
>>  dd->flags &= ~AES_FLAGS_BUSY;
>>
>> +   if (rctx->mode & AES_FLAGS_CIPHERTAIL) {
>> +   if (rctx->mode & AES_FLAGS_ENCRYPT) {
>> +   scatterwalk_map_and_copy(req->info, req->dst,
>> +req->nbytes - ivsize,
>> +ivsize, 0);
>> +   } else {
>> +   memcpy(req->info, rctx->ciphertail, ivsize);
>
>
> why don't we make the same assumption and replace ivsize with
> AES_BLOCK_SIZE? Is there any chance that req->info was allocated with
> less than AES_BLOCK_SIZE size?
>
I do not really know. I'm just getting used to the crypto framework, and the
fact that the iv buffer size is implicit...

>> +   memset(rctx->ciphertail, 0, ivsize);
>
>
> memset to zero is not necessary.
>
OK

>> +   }
>> +   }
>> +
>>  if (dd->is_async)
>>  dd->areq->complete(dd->areq, err);
>>
>> @@ -1071,11 +1089,13 @@ static int atmel_aes_ctr_start(struct
>> atmel_aes_dev *dd)
>>
>>   static int atmel_aes_crypt(struct ablkcipher_request *req, unsigned long
>> mode)
>>   {
>> +   struct crypto_ablkcipher *ablkcipher;
>>  struct atmel_aes_base_ctx *ctx;
>>  struct atmel_aes_reqctx *rctx;
>>  struct atmel_aes_dev *dd;
>>
>> -   ctx = crypto_ablkcipher_ctx(crypto_ablkcipher_reqtfm(req));
>> +   ablkcipher = crypto_ablkcipher_reqtfm(req);
>> +   ctx = crypto_ablkcipher_ctx(ablkcipher);
>
>
> you can initialize these variables at declaration.
>
OK

>>  switch (mode & AES_FLAGS_OPMODE_MASK) {
>>  case AES_FLAGS_CFB8:
>>  ctx->block_size = CFB8_BLOCK_SIZE;
>> @@ -1103,7 +1123,15 @@ static int atmel_aes_crypt(struct
>> ablkcipher_request *req, unsigned long mode)
>>  return -ENODEV;
>>
>>  rctx = ablkcipher_request_ctx(req);
>
>
> while here, please initialize this at declaration.
>
> Also, this one:
> '''
> dd = atmel_aes_find_dev(ctx);
> if (!dd)
> return -ENODEV;
>
> '''
> should be moved at the beginning of the function. If an initialization
> might fail, let's check it as soon as we can, so that we don't waste
> resources.
>
Most of these initializations are in fact structure casts.

>> -   rctx->mode = mode;
>> +
>> +   if (!(mode & AES_FLAGS_ENCRYPT)) {
>> +   int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
>> +
>> +   scatterwalk_map_and_copy(rctx->ciphertail, req->src,
>> +   (req->nbytes - ivsize), ivsize, 0);
>> +   }
>> +   rctx->mode = mode | AES_FLAGS_CIPHERTAIL;
>
>
> saving the last ciphertext block is a requirement for skcipher. This
> flag is redundant, there's no need to use it.
>

The idea regarding the ciphertail flag was because atmel_aes_complete
is used by for regular block crypto (ecb, cbc, ctr, etc.) but also for
aead transfers in gcm and authenc cases, which triggered panics in
my code.

I now realize that casting the areq value to an ablkcipher_req is wrong,
as it can

RE: [PATCH 0/4] add CAAM DMA memcpy driver

2017-10-27 Thread Koul, Vinod 
> On 10/26/2017 1:01 PM, Radu Alexe wrote:
> > This patch-set introduces a new DMA memcpy driver based on the DMA
> > capabilities of the CAAM crypto engine. Because of this dependency the
> > included commits target various parts of the kernel tree.
> I don't see the patches on any of the mail lists.
> If they are moderated, I would expect this to be mentioned in the
> MAINTAINERS file.
> 
> >
> > Patch 1.
> > Since the CAAM DMA driver is a platform driver it is enabled by a new node
> > in the device tree. This commit adds documentation for the device tree
> > bindings.
> >
> > Patch 2.
> > This patch adds the "caam-dma" node in the fsl-ls1012a.dtsi file.
> >
> > Patch 3.
> > This commit adds various capabilities in the JR driver of the CAAM that is
> > used by the CAAM DMA driver.
> >
> > Patch 4.
> > Adds the CAAM DMA memcpy driver.
> >
> > Patch 1 and 3 should be merged by the crypto maintainers, patch 2 by
> > devicetree maintainers and patch 4 by the DMA maintainers.
> I would go with all the patches through the dmaengine tree.

Good call, but no one CCed dmaengine folks on this series!

-- 
~Vinod

Re: [Part2 PATCH v6 13/38] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

2017-10-27 Thread Brijesh Singh 


On 10/27/17 2:56 AM, Borislav Petkov wrote:
> On Thu, Oct 26, 2017 at 03:59:32PM -0500, Brijesh Singh wrote:
>> we can workaround #1 by adding some hooks in sp_pci_init() to invoke the PSP
>> initialization routines after pci_register_driver() is done but #2 can get
>> painful because it will require us calling the SHUTDOWN outside the
>> sp_pci_exit() code flow.
> Ok, do that and init the PSP master and then put the device in UNINIT
> state only in the functions which execute those commands which need the
> device to be in UNINIT state, e.g., wrap the SEV_CMD_FACTORY_RESET glue
> in a command function which does put the device in the UNINIT state as a
> first step.

transiting a platform in UINIT state to handle the FACTORY_RESET can
have a negative consequence.

Consider this scenario:

Process A
-
sev_launch_start(...)

while (count < 1) {
    sev_launch_update(...)
}

sev_launch_finish()
...
...

Process B:
-

sev_factory_reset();


If in order to handle the FACTORY_RESET we  transition a platform in
UINIT state then it will results as unexpected failure from the
sev_launch_update() because the FACTORY_RESET command remove all the
state information created by sev_launch_start() etc. 

I think our design so far is simple, if command require INIT state then
caller executes sev_platform_init(), then command and finish with
sev_platform_shutdown(). If command does not require INIT state, then
simply issue the command. e.g currently, when caller issues
FACTORY_RESET then we pass command directly to PSP and if FW is in INIT
state then FACTORY_RESET returns error (INVALID_STATE/EBUSY) and we
propagate the error code to userspace.  User can retry the command
sometime later when nobody else is using the PSP.


>
> Then, when that function is done, put the device in the mode which the
> other commands would expect it to be in, e.g., INIT state.
>
> This way you'll simplify the whole command flow considerably and won't
> have to "toggle" the device each time and will save yourself a lot of
> time on command execution.
>
> Thx.
>

Re: [PATCH 0/4] add CAAM DMA memcpy driver

2017-10-27 Thread Horia Geantă 
On 10/26/2017 1:01 PM, Radu Alexe wrote:
> This patch-set introduces a new DMA memcpy driver based on the DMA
> capabilities of the CAAM crypto engine. Because of this dependency the
> included commits target various parts of the kernel tree.
I don't see the patches on any of the mail lists.
If they are moderated, I would expect this to be mentioned in the
MAINTAINERS file.

> 
> Patch 1.
> Since the CAAM DMA driver is a platform driver it is enabled by a new node
> in the device tree. This commit adds documentation for the device tree
> bindings.
> 
> Patch 2.
> This patch adds the "caam-dma" node in the fsl-ls1012a.dtsi file.
> 
> Patch 3.
> This commit adds various capabilities in the JR driver of the CAAM that is
> used by the CAAM DMA driver.
> 
> Patch 4.
> Adds the CAAM DMA memcpy driver.
> 
> Patch 1 and 3 should be merged by the crypto maintainers, patch 2 by
> devicetree maintainers and patch 4 by the DMA maintainers.
I would go with all the patches through the dmaengine tree.

Horia

Re: [PATCH] staging: ccree: Fix indentation in ssi_buffer_mgr.c

2017-10-27 Thread Dan Carpenter 
On Thu, Oct 26, 2017 at 06:53:42PM -0700, Stephen Brennan wrote:
> In particular, fixes some over-indented if statement bodies as well as a
> couple lines indented with spaces. checkpatch.pl now reports no warnings
> on this file other than 80 character warnings.
> 
> Signed-off-by: Stephen Brennan 
> ---
> Hello again, hoping these indentation issues are a bit more actionable than 
> the
> line length errors I was trying to fix in my previous patch. Again, thanks in
> advance for taking the time to look!
> 
>  drivers/staging/ccree/ssi_buffer_mgr.c | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c 
> b/drivers/staging/ccree/ssi_buffer_mgr.c
> index dca3ce84d043..7c62255128d5 100644
> --- a/drivers/staging/ccree/ssi_buffer_mgr.c
> +++ b/drivers/staging/ccree/ssi_buffer_mgr.c
> @@ -406,8 +406,8 @@ ssi_aead_handle_config_buf(struct device *dev,
>   sg_init_one(_ctx->ccm_adata_sg, config_data, AES_BLOCK_SIZE + 
> areq_ctx->ccm_hdr_size);
>   if (unlikely(dma_map_sg(dev, _ctx->ccm_adata_sg, 1,
>   DMA_TO_DEVICE) != 1)) {
> - dev_err(dev, "dma_map_sg() config buffer failed\n");
> - return -ENOMEM;
> + dev_err(dev, "dma_map_sg() config buffer failed\n");
> + return -ENOMEM;
>   }
>   dev_dbg(dev, "Mapped curr_buff: dma_address=%pad page=%p addr=%pK 
> offset=%u length=%u\n",
>   _dma_address(_ctx->ccm_adata_sg),
> @@ -435,8 +435,8 @@ static inline int ssi_ahash_handle_curr_buf(struct device 
> *dev,
>   sg_init_one(areq_ctx->buff_sg, curr_buff, curr_buff_cnt);
>   if (unlikely(dma_map_sg(dev, areq_ctx->buff_sg, 1,
>   DMA_TO_DEVICE) != 1)) {
> - dev_err(dev, "dma_map_sg() src buffer failed\n");
> - return -ENOMEM;
> + dev_err(dev, "dma_map_sg() src buffer failed\n");
> + return -ENOMEM;

So far so good.

>   }
>   dev_dbg(dev, "Mapped curr_buff: dma_address=%pad page=%p addr=%pK 
> offset=%u length=%u\n",
>   _dma_address(areq_ctx->buff_sg), sg_page(areq_ctx->buff_sg),
> @@ -1029,10 +1029,10 @@ static inline int 
> ssi_buffer_mgr_prepare_aead_data_mlli(
>* verification is made by CPU compare in order to 
> simplify
>* MAC verification upon request completion
>*/
> -   u32 size_to_skip = req->assoclen;
> + u32 size_to_skip = req->assoclen;
>  
> -   if (areq_ctx->is_gcm4543)
> -   size_to_skip += crypto_aead_ivsize(tfm);
> + if (areq_ctx->is_gcm4543)
> + size_to_skip += crypto_aead_ivsize(tfm);
>  
> ssi_buffer_mgr_copy_scatterlist_portion(
>   dev, areq_ctx->backup_mac, req->src,

But then ssi_buffer_mgr_copy_scatterlist_portion() is still not indented
correctly.

regards,
dan carpenter

Re: [Part2 PATCH v6 13/38] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

2017-10-27 Thread Borislav Petkov 
On Thu, Oct 26, 2017 at 03:59:32PM -0500, Brijesh Singh wrote:
> we can workaround #1 by adding some hooks in sp_pci_init() to invoke the PSP
> initialization routines after pci_register_driver() is done but #2 can get
> painful because it will require us calling the SHUTDOWN outside the
> sp_pci_exit() code flow.

Ok, do that and init the PSP master and then put the device in UNINIT
state only in the functions which execute those commands which need the
device to be in UNINIT state, e.g., wrap the SEV_CMD_FACTORY_RESET glue
in a command function which does put the device in the UNINIT state as a
first step.

Then, when that function is done, put the device in the mode which the
other commands would expect it to be in, e.g., INIT state.

This way you'll simplify the whole command flow considerably and won't
have to "toggle" the device each time and will save yourself a lot of
time on command execution.

Thx.

-- 
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--