Hi,

This is another attempt to come up with an RPMB API for the kernel.
The last discussion of this was in the thread:

  Subject: [RFC PATCH  0/5] RPMB internal and user-space API + WIP virtio-rpmb frontend
  Date: Wed,  3 Mar 2021 13:54:55 +0000
  Message-Id: <20210303135500.24673-1-alex.bennee@linaro.org>

The series provides for the RPMB sub-system, a new chardev API driven
by ioctls and a full multi-block capable virtio-rpmb driver. You can
find a working vhost-user backend in my QEMU branch here:

  https://github.com/stsquad/qemu/commits/virtio/vhost-user-rpmb-v2

The branch is a little messy but I'll be posting a cleaned up version
in the following weeks. The only real changes to the backend is the
multi-block awareness and some tweaks to deal with QEMU internals
handling VirtIO config space messages which weren't previously
exercised. The test.sh script in tools/rpmb works through the various
transactions but isn't comprehensive.

Changes since the last posting:

  - frame construction is mostly back in userspace

  The previous discussion showed there wasn't any appetite for using
  the kernels keyctl() interface so userspace yet again takes
  responsibility for constructing most* frames. Currently these are
  all pure virtio-rpmb frames but the code is written so we can plug
  in additional frame types. The virtio-rpmb driver does some
  validation and in some cases (* read-blocks) constructs the request
  frame in the driver. It would take someone implementing a driver for
  another RPMB device type to see if this makes sense.

  - user-space interface is still split across several ioctls

  Although 3 of the ioctls share the common rpmb_ioc_reqresp_cmd
  structure it does mean things like capacity, write_count and
  read_blocks can have their own structure associated with the
  command.

As before I shall follow up with the QEMU based vhost-user backend and
hopefully a rust-vmm re-implementation. However I've no direct
interest in implementing the interfaces to real hardware. I leave that
to people who have access to such things and are willing to take up
the maintainer burden if this is merged.

Regards,

Alex
    

Alex Bennée (4):
  rpmb: add Replay Protected Memory Block (RPMB) subsystem
  char: rpmb: provide a user space interface
  rpmb: create virtio rpmb frontend driver
  tools rpmb: add RPBM access tool

 .../userspace-api/ioctl/ioctl-number.rst      |    1 +
 MAINTAINERS                                   |    9 +
 drivers/Kconfig                               |    2 +
 drivers/Makefile                              |    1 +
 drivers/rpmb/Kconfig                          |   28 +
 drivers/rpmb/Makefile                         |    9 +
 drivers/rpmb/cdev.c                           |  309 +++++
 drivers/rpmb/core.c                           |  439 +++++++
 drivers/rpmb/rpmb-cdev.h                      |   17 +
 drivers/rpmb/virtio_rpmb.c                    |  518 ++++++++
 include/linux/rpmb.h                          |  182 +++
 include/uapi/linux/rpmb.h                     |   99 ++
 include/uapi/linux/virtio_rpmb.h              |   54 +
 tools/Makefile                                |   16 +-
 tools/rpmb/.gitignore                         |    2 +
 tools/rpmb/Makefile                           |   41 +
 tools/rpmb/key                                |    1 +
 tools/rpmb/rpmb.c                             | 1083 +++++++++++++++++
 tools/rpmb/test.sh                            |   22 +
 19 files changed, 2828 insertions(+), 5 deletions(-)
 create mode 100644 drivers/rpmb/Kconfig
 create mode 100644 drivers/rpmb/Makefile
 create mode 100644 drivers/rpmb/cdev.c
 create mode 100644 drivers/rpmb/core.c
 create mode 100644 drivers/rpmb/rpmb-cdev.h
 create mode 100644 drivers/rpmb/virtio_rpmb.c
 create mode 100644 include/linux/rpmb.h
 create mode 100644 include/uapi/linux/rpmb.h
 create mode 100644 include/uapi/linux/virtio_rpmb.h
 create mode 100644 tools/rpmb/.gitignore
 create mode 100644 tools/rpmb/Makefile
 create mode 100644 tools/rpmb/key
 create mode 100644 tools/rpmb/rpmb.c
 create mode 100755 tools/rpmb/test.sh

-- 
2.30.2

Alex Bennée <alex.bennee@linaro.org> writes:

> Hi,
>
> This is another attempt to come up with an RPMB API for the kernel.
> The last discussion of this was in the thread:

Ping?

Any other comments or reviews? Is there a desire to make other devices
that provide RPMB functionality visible via a common API?

>
>   Subject: [RFC PATCH  0/5] RPMB internal and user-space API + WIP virtio-rpmb frontend
>   Date: Wed,  3 Mar 2021 13:54:55 +0000
>   Message-Id: <20210303135500.24673-1-alex.bennee@linaro.org>
>
> The series provides for the RPMB sub-system, a new chardev API driven
> by ioctls and a full multi-block capable virtio-rpmb driver. You can
> find a working vhost-user backend in my QEMU branch here:
>
>   https://github.com/stsquad/qemu/commits/virtio/vhost-user-rpmb-v2
>
> The branch is a little messy but I'll be posting a cleaned up version
> in the following weeks. The only real changes to the backend is the
> multi-block awareness and some tweaks to deal with QEMU internals
> handling VirtIO config space messages which weren't previously
> exercised. The test.sh script in tools/rpmb works through the various
> transactions but isn't comprehensive.
>
> Changes since the last posting:
>
>   - frame construction is mostly back in userspace
>
>   The previous discussion showed there wasn't any appetite for using
>   the kernels keyctl() interface so userspace yet again takes
>   responsibility for constructing most* frames. Currently these are
>   all pure virtio-rpmb frames but the code is written so we can plug
>   in additional frame types. The virtio-rpmb driver does some
>   validation and in some cases (* read-blocks) constructs the request
>   frame in the driver. It would take someone implementing a driver for
>   another RPMB device type to see if this makes sense.
>
>   - user-space interface is still split across several ioctls
>
>   Although 3 of the ioctls share the common rpmb_ioc_reqresp_cmd
>   structure it does mean things like capacity, write_count and
>   read_blocks can have their own structure associated with the
>   command.
>
> As before I shall follow up with the QEMU based vhost-user backend and
> hopefully a rust-vmm re-implementation. However I've no direct
> interest in implementing the interfaces to real hardware. I leave that
> to people who have access to such things and are willing to take up
> the maintainer burden if this is merged.
>
> Regards,
>
> Alex
>     
>
> Alex Bennée (4):
>   rpmb: add Replay Protected Memory Block (RPMB) subsystem
>   char: rpmb: provide a user space interface
>   rpmb: create virtio rpmb frontend driver
>   tools rpmb: add RPBM access tool
>
>  .../userspace-api/ioctl/ioctl-number.rst      |    1 +
>  MAINTAINERS                                   |    9 +
>  drivers/Kconfig                               |    2 +
>  drivers/Makefile                              |    1 +
>  drivers/rpmb/Kconfig                          |   28 +
>  drivers/rpmb/Makefile                         |    9 +
>  drivers/rpmb/cdev.c                           |  309 +++++
>  drivers/rpmb/core.c                           |  439 +++++++
>  drivers/rpmb/rpmb-cdev.h                      |   17 +
>  drivers/rpmb/virtio_rpmb.c                    |  518 ++++++++
>  include/linux/rpmb.h                          |  182 +++
>  include/uapi/linux/rpmb.h                     |   99 ++
>  include/uapi/linux/virtio_rpmb.h              |   54 +
>  tools/Makefile                                |   16 +-
>  tools/rpmb/.gitignore                         |    2 +
>  tools/rpmb/Makefile                           |   41 +
>  tools/rpmb/key                                |    1 +
>  tools/rpmb/rpmb.c                             | 1083 +++++++++++++++++
>  tools/rpmb/test.sh                            |   22 +
>  19 files changed, 2828 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/rpmb/Kconfig
>  create mode 100644 drivers/rpmb/Makefile
>  create mode 100644 drivers/rpmb/cdev.c
>  create mode 100644 drivers/rpmb/core.c
>  create mode 100644 drivers/rpmb/rpmb-cdev.h
>  create mode 100644 drivers/rpmb/virtio_rpmb.c
>  create mode 100644 include/linux/rpmb.h
>  create mode 100644 include/uapi/linux/rpmb.h
>  create mode 100644 include/uapi/linux/virtio_rpmb.h
>  create mode 100644 tools/rpmb/.gitignore
>  create mode 100644 tools/rpmb/Makefile
>  create mode 100644 tools/rpmb/key
>  create mode 100644 tools/rpmb/rpmb.c
>  create mode 100755 tools/rpmb/test.sh


-- 
Alex Bennée

Hi Alex,

Thanks for this unified RPMB interface, I wanted to verify this on our
UFS, it seems you didn't add the UFS access interface in this version 
from your userspace tools, right?


Kind regards,
Bean

Bean Huo <huobean@gmail.com> writes:

> Hi Alex,
>
> Thanks for this unified RPMB interface, I wanted to verify this on our
> UFS, it seems you didn't add the UFS access interface in this version 
> from your userspace tools, right?

No I didn't but it should be easy enough to add some function pointer
redirection everywhere one of the op_* functions calls a vrpmb_*
function. Do you already have a UFS RPMB device driver?

-- 
Alex Bennée

On Tue, 2022-04-05 at 16:43 +0100, Alex Bennée wrote:
> 
> Bean Huo <huobean@gmail.com> writes:
> 
> > Hi Alex,
> > 
> > Thanks for this unified RPMB interface, I wanted to verify this on
> > our
> > UFS, it seems you didn't add the UFS access interface in this
> > version 
> > from your userspace tools, right?
> 
> No I didn't but it should be easy enough to add some function pointer
> redirection everywhere one of the op_* functions calls a vrpmb_*
> function. Do you already have a UFS RPMB device driver?
> 

Hi Alex,
Thanks for your feedback.

We now access UFS RPMB through the RPMB LUN BSG device, RPMB is a well-
known LU and we have a userspace tool to access it.

I see that if we're going to use your interface, "static struct
rpmb_ops" should be registered from a lower-level driver, for example
in a UFS driver, yes there should be no problem with this registration,
but I don't know with the current way Compared, what are the advantages
to add a driver. maybe the main advantage is that we will have an
unified user space tool for RPMB. right?

Kind regards,
Bean

Bean Huo <huobean@gmail.com> writes:

> On Tue, 2022-04-05 at 16:43 +0100, Alex Bennée wrote:
>> 
>> Bean Huo <huobean@gmail.com> writes:
>> 
>> > Hi Alex,
>> > 
>> > Thanks for this unified RPMB interface, I wanted to verify this on
>> > our
>> > UFS, it seems you didn't add the UFS access interface in this
>> > version 
>> > from your userspace tools, right?
>> 
>> No I didn't but it should be easy enough to add some function pointer
>> redirection everywhere one of the op_* functions calls a vrpmb_*
>> function. Do you already have a UFS RPMB device driver?
>> 
>
> Hi Alex,
> Thanks for your feedback.
>
> We now access UFS RPMB through the RPMB LUN BSG device, RPMB is a well-
> known LU and we have a userspace tool to access it.
>
> I see that if we're going to use your interface, "static struct
> rpmb_ops" should be registered from a lower-level driver, for example
> in a UFS driver, yes there should be no problem with this registration,
> but I don't know with the current way Compared, what are the advantages
> to add a driver. maybe the main advantage is that we will have an
> unified user space tool for RPMB. right?

Pretty much. The main issue for virtio-rpmb is it doesn't really fit
neatly into the block stack because all it does is the RPMB part so a
non-block orientate API makes sense.

Can you point be to where the UFS driver does it's current RPMB stuff?

>
> Kind regards,
> Bean


-- 
Alex Bennée

On Wed, 2022-04-06 at 12:22 +0100, Alex Bennée wrote:
> 
> Bean Huo <huobean@gmail.com> writes:
> 
> > On Tue, 2022-04-05 at 16:43 +0100, Alex Bennée wrote:
> > > 
> > > Bean Huo <huobean@gmail.com> writes:
> > > 
> > > > Hi Alex,
> > > > 
> > > > Thanks for this unified RPMB interface, I wanted to verify this
> > > > on
> > > > our
> > > > UFS, it seems you didn't add the UFS access interface in this
> > > > version 
> > > > from your userspace tools, right?
> > > 
> > > No I didn't but it should be easy enough to add some function
> > > pointer
> > > redirection everywhere one of the op_* functions calls a vrpmb_*
> > > function. Do you already have a UFS RPMB device driver?
> > > 
> > 
> > Hi Alex,
> > Thanks for your feedback.
> > 
> > We now access UFS RPMB through the RPMB LUN BSG device, RPMB is a
> > well-
> > known LU and we have a userspace tool to access it.
> > 
> > I see that if we're going to use your interface, "static struct
> > rpmb_ops" should be registered from a lower-level driver, for
> > example
> > in a UFS driver, yes there should be no problem with this
> > registration,
> > but I don't know with the current way Compared, what are the
> > advantages
> > to add a driver. maybe the main advantage is that we will have an
> > unified user space tool for RPMB. right?
> 
> Pretty much. The main issue for virtio-rpmb is it doesn't really fit
> neatly into the block stack because all it does is the RPMB part so a
> non-block orientate API makes sense.
> 
> Can you point be to where the UFS driver does it's current RPMB
> stuff?
> 

It's the SCSI BSG driver, in fact, we don't have a dedicated UFS RPMB
driver in the kernel. RPMB is a well known LU, we are using userspace
tools to issue SCSI commands directly to the UFS RPMB LU via ioctl()
from the BSG device node in the /dev/sg/ folder.

Here is the BSG part of the code in the userspace tools:

        io_hdr_v4.guard = 'Q';                                        
        io_hdr_v4.protocol = BSG_PROTOCOL_SCSI;                       
        io_hdr_v4.subprotocol = BSG_SUB_PROTOCOL_SCSI_CMD;            
        io_hdr_v4.response = (__u64)sense_buffer;                     
        io_hdr_v4.max_response_len = SENSE_BUFF_LEN;                  
        io_hdr_v4.request_len = cmd_len;                              
        io_hdr_v4.request = (__u64)cdb;                               
                                                                                                                          
                                                                                 
        ioctl(fd, SG_IO, &io_hdr_v4))
...


> > 
> > Kind regards,
> > Bean
> 
> 

On 4/6/22 10:19, Bean Huo wrote:
> On Wed, 2022-04-06 at 12:22 +0100, Alex Bennée wrote:
>>
>> Bean Huo <huobean@gmail.com> writes:
>>
>>> On Tue, 2022-04-05 at 16:43 +0100, Alex Bennée wrote:
>>>>
>>>> Bean Huo <huobean@gmail.com> writes:
>>>>
>>>>> Hi Alex,
>>>>>
>>>>> Thanks for this unified RPMB interface, I wanted to verify this
>>>>> on
>>>>> our
>>>>> UFS, it seems you didn't add the UFS access interface in this
>>>>> version
>>>>> from your userspace tools, right?
>>>>
>>>> No I didn't but it should be easy enough to add some function
>>>> pointer
>>>> redirection everywhere one of the op_* functions calls a vrpmb_*
>>>> function. Do you already have a UFS RPMB device driver?
>>>>
>>>
>>> Hi Alex,
>>> Thanks for your feedback.
>>>
>>> We now access UFS RPMB through the RPMB LUN BSG device, RPMB is a
>>> well-
>>> known LU and we have a userspace tool to access it.
>>>
>>> I see that if we're going to use your interface, "static struct
>>> rpmb_ops" should be registered from a lower-level driver, for
>>> example
>>> in a UFS driver, yes there should be no problem with this
>>> registration,
>>> but I don't know with the current way Compared, what are the
>>> advantages
>>> to add a driver. maybe the main advantage is that we will have an
>>> unified user space tool for RPMB. right?
>>
>> Pretty much. The main issue for virtio-rpmb is it doesn't really fit
>> neatly into the block stack because all it does is the RPMB part so a
>> non-block orientate API makes sense.
>>
>> Can you point be to where the UFS driver does it's current RPMB
>> stuff?
>>
> 
> It's the SCSI BSG driver, in fact, we don't have a dedicated UFS RPMB
> driver in the kernel. RPMB is a well known LU, we are using userspace
> tools to issue SCSI commands directly to the UFS RPMB LU via ioctl()
> from the BSG device node in the /dev/sg/ folder.
> 
> Here is the BSG part of the code in the userspace tools:
> 
>          io_hdr_v4.guard = 'Q';
>          io_hdr_v4.protocol = BSG_PROTOCOL_SCSI;
>          io_hdr_v4.subprotocol = BSG_SUB_PROTOCOL_SCSI_CMD;
>          io_hdr_v4.response = (__u64)sense_buffer;
>          io_hdr_v4.max_response_len = SENSE_BUFF_LEN;
>          io_hdr_v4.request_len = cmd_len;
>          io_hdr_v4.request = (__u64)cdb;
>                                                                                                                            
>                                                                                   
>          ioctl(fd, SG_IO, &io_hdr_v4))

Hi Bean,

I'm not sure where the above comes from? The Android RPMB client uses 
slightly different code. Additionally, the retry loop around the 
submission of SG/IO commands is very important. See also the 
check_sg_io_hdr() call in send_ufs_rpmb_req(). See also 
https://cs.android.com/android/platform/superproject/+/master:system/core/trusty/storage/proxy/rpmb.c

Thanks,

Bart.

On Wed, 2022-04-06 at 10:32 -0700, Bart Van Assche wrote:
> > It's the SCSI BSG driver, in fact, we don't have a dedicated UFS
> > RPMB
> > driver in the kernel. RPMB is a well known LU, we are using
> > userspace
> > tools to issue SCSI commands directly to the UFS RPMB LU via
> > ioctl()
> > from the BSG device node in the /dev/sg/ folder.
> > 
> > Here is the BSG part of the code in the userspace tools:
> > 
> >           io_hdr_v4.guard = 'Q';
> >           io_hdr_v4.protocol = BSG_PROTOCOL_SCSI;
> >           io_hdr_v4.subprotocol = BSG_SUB_PROTOCOL_SCSI_CMD;
> >           io_hdr_v4.response = (__u64)sense_buffer;
> >           io_hdr_v4.max_response_len = SENSE_BUFF_LEN;
> >           io_hdr_v4.request_len = cmd_len;
> >           io_hdr_v4.request = (__u64)cdb;
> >                                                                    
> >                                                          
> >                                                                    
> >                 
> >           ioctl(fd, SG_IO, &io_hdr_v4))
> 
> Hi Bean,
> 
> I'm not sure where the above comes from? The Android RPMB client uses
> slightly different code. Additionally, the retry loop around the 
> submission of SG/IO commands is very important. See also the 
> check_sg_io_hdr() call in send_ufs_rpmb_req(). See also 
> https://cs.android.com/android/platform/superproject/+/master:system/core/trusty/storage/proxy/rpmb.c
> 
> 
Bart,

It is from the ufs-utils.

So, do you vote to add the UFS RPMB driver based on this new framework
to resolve this conflict?


Kind regards,
Bean

> Thanks,
> 


> Bart.

On 4/6/22 11:12, Bean Huo wrote:
> It is from the ufs-utils.
> 
> So, do you vote to add the UFS RPMB driver based on this new framework
> to resolve this conflict?

Are any applications using the RPMB code from ufs-utils? It seems to me 
that the ufs-utils code doe not handle SCSI unit attentions correctly. 
If a POWER ON unit attention is received as reply to a SECURITY PROTOCOL 
OUT transaction, the write counter should be reread instead of retrying 
the SECURITY PROTOCOL OUT command with the same write counter.

Regarding adding a UFS RPMB driver: that seems useful to me since 
multiple applications make use of the UFS RPMB functionality. My 
understanding is that currently storageproxyd multiplexes UFS RPMB 
accesses in Android.

Thanks,

Bart.

On Wed, 2022-04-06 at 13:20 -0700, Bart Van Assche wrote:
> On 4/6/22 11:12, Bean Huo wrote:
> > It is from the ufs-utils.
> > 
> > So, do you vote to add the UFS RPMB driver based on this new
> > framework
> > to resolve this conflict?
> 
> Are any applications using the RPMB code from ufs-utils? It seems to
> me 
> that the ufs-utils code doe not handle SCSI unit attentions
> correctly. 
> If a POWER ON unit attention is received as reply to a SECURITY
> PROTOCOL 
> OUT transaction, the write counter should be reread instead of
> retrying 
> the SECURITY PROTOCOL OUT command with the same write counter.
> 

Not much sure how customers use this tool, based on my little
information from the field, the customer developed their own RPMB code
in the application. Here utils code is good example for them to study
and verify RPMB functinalities.

> Regarding adding a UFS RPMB driver: that seems useful to me since 
> multiple applications make use of the UFS RPMB functionality. My 
> understanding is that currently storageproxyd multiplexes UFS RPMB 
> accesses in Android.
> 

I have the same opinion with you, if we have an unified RPMB access
interface, and adding RPMB driver in the kernel, thus is better to
manage RPMB.

Kind regards,
Bean

> Thanks,
> 
> Bart.

> 
> Can you point be to where the UFS driver does it's current RPMB
> stuff?
> 

If you want to understand the UFS RPMB functionality/command sequence,
you can refer to the ufs-utils tool, it is much like mmc-utils.

https://github.com/westerndigitalcorporation/ufs-utils/blob/dev/ufs_rpmb.c

> > 
> > Kind regards,
> > Bean
> 
>