All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Grzegorz Kołodziejczyk" <grzegorz.kolodziejczyk@codecoup.pl>
To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: linux-bluetooth@vger.kernel.org
Subject: Re: [RFC 1/2] shared/gatt-server: Request authorization for prepare writes.
Date: Wed, 02 May 2018 13:31:01 +0000	[thread overview]
Message-ID: <CALevQMa0KqNd46MBuL6PPTUodXrWOyC4U_J-xgv-KQ1CBNH3HQ@mail.gmail.com> (raw)
In-Reply-To: <CABBYNZKbHnKF_GbFq3qQpzjtb0s-EwdR=9RwMb34r-T7D0kT+Q@mail.gmail.com>

Hi Luiz,
=C5=9Br., 2 maj 2018 o 13:33 Luiz Augusto von Dentz <luiz.dentz@gmail.com>
napisa=C5=82(a):

> Hi Grzegorz,
> On Wed, May 2, 2018 at 1:09 PM Grzegorz Kolodziejczyk <
> grzegorz.kolodziejczyk@codecoup.pl> wrote:

> > This patch adds gatt-server possibility to request authorization from
> > application if needed and previously wasn't authorized. Authorization i=
s
> > requested by sending write request with no data to application.
> > ---
> >   doc/gatt-api.txt         |  8 ++++++
> >   src/gatt-database.c      |  6 +++++
> >   src/shared/gatt-server.c | 64
> +++++++++++++++++++++++++++++++++++++++++-------
> >   3 files changed, 69 insertions(+), 9 deletions(-)

> > diff --git a/doc/gatt-api.txt b/doc/gatt-api.txt
> > index 0f1cc9029..b9bdc9352 100644
> > --- a/doc/gatt-api.txt
> > +++ b/doc/gatt-api.txt
> > @@ -251,6 +251,14 @@ Properties string UUID [read-only]
> >                                  "secure-read" (Server only)
> >                                  "secure-write" (Server only)

> Documentation changes should be split in a different patch.

> > +               boolean Authorized [read-only, optional] (Server only)
> > +
> > +                       True, if read/write operation was previously
> authorized
> > +                       and attribute requires authorization. If no
> Authorized
> > +                       property is available along with characteristic
> or this
> > +                       property is set to true, then daemon don't send
> empty
> > +                       write request for authorization.

> I guess we are missing the authorization flag, otherwise the daemon canno=
t
> tell if authorization is required on not, or we assume it is always
> required if Authorized is set to false? In that case, we should make it
If there is no authorization request then there is no property on client
(chrc_get_authorized_exists checks for prop existence).
There are 3 ways of handling this property:
1) No property - means that writing behaves as before without issuing
authorization request part, only gatt database asks for property existence.
2) Property set to False - means that there is authorization request for
preparing write this property, once authorized it sets to True
3) Property set to True - means that write was previously authorized and
any continous (offset > 0) write can be issued. If write will be issued
with offset =3D 0, new authorization request will be required.
> clearer in the documentation. Perhaps we should make the property state i=
f
> the authorization is required like boolean Authorization, if it is not
> present it would be assumed to be false meaning authorization is not
> required. We should also state how it affects the calls stating that a
> prepare write would cause WriteValue to be called with empty data if
> authorization is required, and perhaps we should attempt to call
WriteValue
> just once per procedure which means the authorization would be valid unti=
l
> execute, or we change the property to be string that can assume the
values:
> always, once, never. Btw, I don't think we can support Authorization with
Every time write request will be issued with offset =3D 0 - application wil=
l
be asked for authorization - that was my assumption.
> AcquireWrite since there is no reply to writes on the pipe, so we might a=
s
> well document that those properties cannot be used together.
Right, I'll update it in next version.


> >   Characteristic Descriptors hierarchy
> >   =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

> > diff --git a/src/gatt-database.c b/src/gatt-database.c
> > index 0ac5b75b0..419db909c 100644
> > --- a/src/gatt-database.c
> > +++ b/src/gatt-database.c
> > @@ -2614,6 +2614,12 @@ static void chrc_write_cb(struct
gatt_db_attribute
> *attrib,
> >                  goto fail;
> >          }

> > +       if (offset && g_dbus_proxy_get_property(chrc->proxy,
"Authorized",
> > +                                                               &iter))
{
> > +               gatt_db_attribute_write_result(attrib, id, 0);
> > +               return;
> > +       }

> What is the logic behind checking the offset? Is this check here because
> the prepare writes are now passed up the stack? Afaik the offset can be
set
> to 0 even with prepare write which means we should check the opcode
instead
> which might be required for execute as well otherwise nothing that has
> offset set will be ever written to the application.

> >          if (chrc->write_io) {
> >                  if (pipe_io_send(chrc->write_io, value, len) < 0) {
> >                          error("Unable to write: %s", strerror(errno));
> > diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> > index 4b554f665..cdade76f8 100644
> > --- a/src/shared/gatt-server.c
> > +++ b/src/shared/gatt-server.c
> > @@ -1208,6 +1208,45 @@ static bool store_prep_data(struct bt_gatt_serve=
r
> *server,
> >          return prep_data_new(server, handle, offset, length, value);
> >   }

> > +struct prep_write_complete_data {
> > +       void *pdu;
> > +       uint16_t length;
> > +       struct bt_gatt_server *server;
> > +};
> > +
> > +static void prep_write_complete_cb(struct gatt_db_attribute *attr, int
> err,
> > +                                                               void
> *user_data)
> > +{
> > +       struct prep_write_complete_data *pwcd =3D user_data;
> > +       uint16_t handle =3D 0;
> > +       uint16_t offset;
> > +
> > +       handle =3D get_le16(pwcd->pdu);
> > +
> > +       if (err) {
> > +               bt_att_send_error_rsp(pwcd->server->att,
> > +                                       BT_ATT_OP_PREP_WRITE_REQ,
handle,
> err);
> > +               free(pwcd->pdu);
> > +               free(pwcd);
> > +
> > +               return;
> > +       }
> > +
> > +       offset =3D get_le16(pwcd->pdu + 2);
> > +
> > +       if (!store_prep_data(pwcd->server, handle, offset, pwcd->length
-
> 4,
> > +                                               &((uint8_t *)
> pwcd->pdu)[4]))
> > +               bt_att_send_error_rsp(pwcd->server->att,
> > +                                       BT_ATT_OP_PREP_WRITE_RSP,
handle,
> > +
> BT_ATT_ERROR_INSUFFICIENT_RESOURCES);
> > +
> > +       bt_att_send(pwcd->server->att, BT_ATT_OP_PREP_WRITE_RSP,
> pwcd->pdu,
> > +                                               pwcd->length, NULL,
NULL,
> NULL);
> > +
> > +       free(pwcd->pdu);
> > +       free(pwcd);
> > +}
> > +
> >   static void prep_write_cb(uint8_t opcode, const void *pdu,
> >                                          uint16_t length, void
*user_data)
> >   {
> > @@ -1215,7 +1254,8 @@ static void prep_write_cb(uint8_t opcode, const
> void *pdu,
> >          uint16_t handle =3D 0;
> >          uint16_t offset;
> >          struct gatt_db_attribute *attr;
> > -       uint8_t ecode;
> > +       struct prep_write_complete_data *pwcd;
> > +       uint8_t ecode, status;

> >          if (length < 4) {
> >                  ecode =3D BT_ATT_ERROR_INVALID_PDU;
> > @@ -1245,15 +1285,21 @@ static void prep_write_cb(uint8_t opcode, const
> void *pdu,
> >          if (ecode)
> >                  goto error;

> > -       if (!store_prep_data(server, handle, offset, length - 4,
> > -                                               &((uint8_t *) pdu)[4]))
{
> > -               ecode =3D BT_ATT_ERROR_INSUFFICIENT_RESOURCES;
> > -               goto error;
> > -       }
> > +       pwcd =3D new0(struct prep_write_complete_data, 1);
> > +       pwcd->pdu =3D malloc(length);
> > +       memcpy(pwcd->pdu, pdu, length);
> > +       pwcd->length =3D length;
> > +       pwcd->server =3D server;

> > -       bt_att_send(server->att, BT_ATT_OP_PREP_WRITE_RSP, pdu, length,
> NULL,
> > -                                                               NULL,
> NULL);
> > -       return;
> > +       status =3D gatt_db_attribute_write(attr, offset, NULL, 0,
> > +
BT_ATT_OP_PREP_WRITE_REQ,
> > +                                               server->att,
> > +                                               prep_write_complete_cb,
> pwcd);
> > +       if (status)
> > +               return;
> > +
> > +       ecode =3D BT_ATT_ERROR_UNLIKELY;

> >   error:
> >          bt_att_send_error_rsp(server->att, opcode, handle, ecode);
> > --
> > 2.13.6

> > --
> > To unsubscribe from this list: send the line "unsubscribe
> linux-bluetooth" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html



> --
> Luiz Augusto von Dentz
Grzegorz Ko=C5=82odziejczyk

  reply	other threads:[~2018-05-02 13:31 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-02  9:17 [RFC 0/2] Add support for prepare writes authorization Grzegorz Kolodziejczyk
2018-05-02  9:17 ` [RFC 1/2] shared/gatt-server: Request authorization for prepare writes Grzegorz Kolodziejczyk
2018-05-02 11:32   ` Luiz Augusto von Dentz
2018-05-02 13:31     ` Grzegorz Kołodziejczyk [this message]
2018-05-03  9:54       ` Luiz Augusto von Dentz
2018-05-02  9:17 ` [RFC 2/2] client: Add authorized property handling to characteristic attribute Grzegorz Kolodziejczyk
2018-05-02 10:42   ` Luiz Augusto von Dentz
2018-05-02 13:06     ` Grzegorz Kołodziejczyk
2018-05-03  9:58       ` Luiz Augusto von Dentz
2018-05-04 14:03         ` Grzegorz Kołodziejczyk

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CALevQMa0KqNd46MBuL6PPTUodXrWOyC4U_J-xgv-KQ1CBNH3HQ@mail.gmail.com \
    --to=grzegorz.kolodziejczyk@codecoup.pl \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=luiz.dentz@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.