On 09/10/2018 11:09 AM, Jean Delvare wrote:
> Functions qeth_get_ipa_msg and qeth_get_ipa_cmd_name are modifying
> the last member of global arrays without any locking that I can see.
> If two instances of either function are running at the same time,
> it could cause a race ultimately leading to an array overrun (the
> contents of the last entry of the array is the only guarantee that
> the loop will ever stop).
>
> Performing the lookups without modifying the arrays is admittedly
> slower (two comparisons per iteration instead of one) but these
> are operations which are rare (should only be needed in error
> cases or when debugging, not during successful operation) and it
> seems still less costly than introducing a mutex to protect the
> arrays in question.
>
> As a side bonus, it allows us to declare both arrays as const data.
>
> Signed-off-by: Jean Delvare <jdelvare@xxxxxxx>
> Cc: Julian Wiedmann <jwi@xxxxxxxxxxxxx>
> Cc: Ursula Braun <ubraun@xxxxxxxxxxxxx>
> Cc: Martin Schwidefsky <schwidefsky@xxxxxxxxxx>
> Cc: Heiko Carstens <heiko.carstens@xxxxxxxxxx>
> ---
> Note: build-tested only.
> Note: applies on top of zhong jiang's patch which makes use of
> ARRAY_SIZE.
>
> drivers/s390/net/qeth_core_main.c | 2 +-
> drivers/s390/net/qeth_core_mpc.c | 31 ++++++++++++++++---------------
> drivers/s390/net/qeth_core_mpc.h | 4 ++--
> 3 files changed, 19 insertions(+), 18 deletions(-)
>
> --- linux-4.19-rc2.orig/drivers/s390/net/qeth_core_mpc.c 2018-09-10 11:00:58.028833294 +0200
> +++ linux-4.19-rc2/drivers/s390/net/qeth_core_mpc.c 2018-09-10 11:04:10.806082673 +0200
> @@ -148,10 +148,10 @@ EXPORT_SYMBOL_GPL(IPA_PDU_HEADER);
>
> struct ipa_rc_msg {
> enum qeth_ipa_return_codes rc;
> - char *msg;
> + const char *msg;
> };
>
> -static struct ipa_rc_msg qeth_ipa_rc_msg[] = {
> +static const struct ipa_rc_msg qeth_ipa_rc_msg[] = {
> {IPA_RC_SUCCESS, "success"},
> {IPA_RC_NOTSUPP, "Command not supported"},
> {IPA_RC_IP_TABLE_FULL, "Add Addr IP Table Full - ipv6"},
> @@ -219,22 +219,23 @@ static struct ipa_rc_msg qeth_ipa_rc_msg
>
>
>
> -char *qeth_get_ipa_msg(enum qeth_ipa_return_codes rc)
> +const char *qeth_get_ipa_msg(enum qeth_ipa_return_codes rc)
> {
> - int x = 0;
> - qeth_ipa_rc_msg[ARRAY_SIZE(qeth_ipa_rc_msg) - 1].rc = rc;
> - while (qeth_ipa_rc_msg[x].rc != rc)
> - x++;
> + int x;
> +
> + for (x = 0; x < ARRAY_SIZE(qeth_ipa_rc_msg) - 1; x++)
> + if (qeth_ipa_rc_msg[x].rc == rc)
> + return qeth_ipa_rc_msg[x].msg;
> return qeth_ipa_rc_msg[x].msg;
> }
>
>
> struct ipa_cmd_names {
> enum qeth_ipa_cmds cmd;
> - char *name;
> + const char *name;
> };
>
> -static struct ipa_cmd_names qeth_ipa_cmd_names[] = {
> +static const struct ipa_cmd_names qeth_ipa_cmd_names[] = {
> {IPA_CMD_STARTLAN, "startlan"},
> {IPA_CMD_STOPLAN, "stoplan"},
> {IPA_CMD_SETVMAC, "setvmac"},
> @@ -263,14 +264,14 @@ static struct ipa_cmd_names qeth_ipa_cmd
> {IPA_CMD_REGISTER_LOCAL_ADDR, "register_local_addr"},
> {IPA_CMD_UNREGISTER_LOCAL_ADDR, "unregister_local_addr"},
> {IPA_CMD_ADDRESS_CHANGE_NOTIF, "address_change_notification"},
> - {IPA_CMD_UNKNOWN, "unknown"},
Why is this line removed? Doesn't the last line of function qeth_get_ipa_cmd_name()
still refer to this line?
And if IPA_CMD_UNKNOWN is really removed, then make sure its definition is removed
from qeth_core_mpc.h as well.
> };
>
> -char *qeth_get_ipa_cmd_name(enum qeth_ipa_cmds cmd)
> +const char *qeth_get_ipa_cmd_name(enum qeth_ipa_cmds cmd)
> {
> - int x = 0;
> - qeth_ipa_cmd_names[ARRAY_SIZE(qeth_ipa_cmd_names) - 1].cmd = cmd;
> - while (qeth_ipa_cmd_names[x].cmd != cmd)
> - x++;
> + int x;
> +
> + for (x = 0; x < ARRAY_SIZE(qeth_ipa_cmd_names) - 1; x++)
> + if (qeth_ipa_cmd_names[x].cmd == cmd)
> + return qeth_ipa_cmd_names[x].name;
> return qeth_ipa_cmd_names[x].name;
> }
> --- linux-4.19-rc2.orig/drivers/s390/net/qeth_core_main.c 2018-09-10 11:00:58.028833294 +0200
> +++ linux-4.19-rc2/drivers/s390/net/qeth_core_main.c 2018-09-10 11:03:50.038825085 +0200
> @@ -609,7 +609,7 @@ static void qeth_put_reply(struct qeth_r
> static void qeth_issue_ipa_msg(struct qeth_ipa_cmd *cmd, int rc,
> struct qeth_card *card)
> {
> - char *ipa_name;
> + const char *ipa_name;
> int com = cmd->hdr.command;
> ipa_name = qeth_get_ipa_cmd_name(com);
> if (rc)
> --- linux-4.19-rc2.orig/drivers/s390/net/qeth_core_mpc.h 2018-09-10 11:00:58.028833294 +0200
> +++ linux-4.19-rc2/drivers/s390/net/qeth_core_mpc.h 2018-09-10 11:03:50.038825085 +0200
> @@ -797,8 +797,8 @@ enum qeth_ipa_arp_return_codes {
> QETH_IPA_ARP_RC_Q_NO_DATA = 0x0008,
> };
>
> -extern char *qeth_get_ipa_msg(enum qeth_ipa_return_codes rc);
> -extern char *qeth_get_ipa_cmd_name(enum qeth_ipa_cmds cmd);
> +extern const char *qeth_get_ipa_msg(enum qeth_ipa_return_codes rc);
> +extern const char *qeth_get_ipa_cmd_name(enum qeth_ipa_cmds cmd);
>
> #define QETH_SETASS_BASE_LEN (sizeof(struct qeth_ipacmd_hdr) + \
> sizeof(struct qeth_ipacmd_setassparms_hdr))
>
